Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

chore: upgrade test studios to react 19, remove next studios #8178

Merged
merged 1 commit into from
Jan 6, 2025

Conversation

stipsan
Copy link
Member

@stipsan stipsan commented Jan 3, 2025

Description

Moves the test-studio, efps studio, and e2e studio, to react 19 at runtime.
Since test-studio is now on react 19 we no longer need the test-next-studio, so it's removed. After this merges I'll disconnect the vercel project from the repo so it stops adding failing checks to PRs, while historical deployments that documents our react 19 adoption and fixes are preserved and remain accessible.

Also thanks to being on 19 it's possible to run Million Lint without scoping it to exclude files that are transformed by React Compiler. The data collected is massive, and requires tons of RAM and CPU to profile. Thus it's available on a new command: pnpm dev:million-lint-everything. It has so much useful data:
Screenshot 2025-01-03 at 18 47 01

I couldn't help myself and fixed a few issues surfaced by it.

What to review

It seems like a massive effort to have the E2E testing suite run on both react 18 and 19, so I opted to run it on 19. If anyone has good ideas on how to make this happen without massive complexity or doubling CI wait time on PRs, feel welcome to contribute!

Should I add a pnpm dev:react-18 command that lets us easily run the test studio on 18, in case react 18 regressions happen in the future? That feels like a good fast follow up to this one :)
I've left vitest and playwright component test suites alone in this PR as it requires more planning in order to figure out how we can test on both react 19 and 18 in a good way, without introducing flake or doubling CI wait time.

Why it makes more sense to have 19 as the day-to-day baseline over 18

While we can setup E2E, integration tests, playwright, CLI tests, and such to run the same suites on both majors of React, we can't demand that of humans.
Time is finite, and if you give engineers the option to run both majors they'll pick one and rarely test the other major.
Currently the majority run 18, as that's what pnpm dev pulls up, and what E2E runs. pnpm dev:next-studio grants access to the same test studio on 19, but has historically only been used when investigating bug reports of something not working on 19. If we were to add a pnpm dev:react-19 command that runs on our vite setup instead of next.js, it's unlikely that'll be used outside of triaging bug reports from react 19 users.

Story time on why it's called `test-next-studio` and why we it has had its own studio deployment.

It's called test-next-studio because those reports typically came from users on Next.js that embedded their studios (a common pattern for applications that has live preview, like presentation tool), where App Router basically have always used React 19 (or to be exact, canary versions of react) regardless of the react version installed in the userland package.json.

Because of the popularity of embedding studios, every time a breaking change in React where introduced to its canary channel it affected our customers, and we had to fix them.
Testing the same version of React as the one Next.js is using isn't as simple as adopting canary release channels of react. Next.js has its own bundler (webpack and turbopack), we're using vite. Their setup includes SSR and hydration, our setup is just a client side rendered app.

It's far easier to just use Next.js and embed the studio directly and it gave us an easy way to test what ultimately became React 19 stable.

It's also worth mentioning that running pnpm dev:next-studio, and even pnpm dev:turbo-studio which is equivalent to next dev --turbopack, is much much slower than our vite based tooling so even if a Sanity engineer preferred testing on react 19 strict mode, it's not worth the much longer feedback loop and time spent waiting for the studio to hot reload.

There are valid concerns for making 19 the new baseline, of course, as it introduces the risk of accidentally breaking studios on 18. Let's break it down in a pros/cons list:

  • React 19 as baseline
    • Pros
      • The improved Strict Mode also helps catch issues that affect React 18 users
      • High confidence in Studio fully supporting React 19, issues are caught early, regression risks much lower.
      • We can use features that support progressive enhancement, for example useDeferredValue has a second parameter on react 19 that sets the initial value on first render, and can be used to do less work on render on 19, while 18 works the same as before.
      • Most features that are 19-only are typically easy to catch. For example by running integration tests on both 18 and 19 we'll catch accidental usage of React.use and other APIs not available to 18. Most of it can be caught with tooling like ESLint a swell.
    • Cons
      • We trade React 18 confidence in new code, as some features in 19 are subtle and more difficult to discover. Even with Sanity engineers having high awareness of what APIs we can't use, we might pull in third party npm libraries that make use of APIs in ways that can only be caught at runtime on 18. For example 19 can send ref as a regular prop, removing requirement to use React.forwardRef, and accidental ref cleanup functions can be difficult to spot.
  • React 18 as baseline
    • Pros
      • High confidence in new code and refactors of legacy code remain stable for react 18 users.
      • Accidental usage of react 19-only APIs is simply not a thing.
    • Cons
      • Difficult to prevent new code breaking in ways are best surfaced by 19 StrictMode.
      • React 19 has more concurrent rendering functionality (for example its assets loading can lead to a component suspending), and is planned to introduce new functionality where it's more important than ever before that all render functions, useMemo, useCallback, are pure and free of side-effects that should be in a useEffect. For example the Offscreen/Activity API lets components progressively render in the background.. While we're on 18 we're stuck with a StrictMode that isn't fully equipped to help us ensure the Studio won't break if these features are applied in custom userland code.
      • We'll always be reactive with our 19 support, causing frustration with our users that are expecting us to be proactive.

TL;DR

With 19 as our baseline we still have a lot of tools in our disposal for preserving React 18 support, while delivering an excellent experience for our users on 19.
That's simply not the case if we remain on 18. Its Strict Mode doesn't help us make sure our code is safe and fully supports concurrent features on 18, nor on 19, and it forces us to remain reactive in our support for 19.

Testing

The E2E testing suite still pass, locally and on the CI. Same for the vercel deployment and manual testing.
Running pnpm dev, pnpm dev:million-lint or pnpm dev:million-lint-everything, should let you use http://localhost:3333 like before, but now on React 19 if you use react devtools to inspect the page.

Notes for release

N/A

Copy link

vercel bot commented Jan 3, 2025

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
page-building-studio ✅ Ready (Inspect) Visit Preview 💬 Add feedback Jan 6, 2025 1:49pm
performance-studio ✅ Ready (Inspect) Visit Preview 💬 Add feedback Jan 6, 2025 1:49pm
test-next-studio ❌ Failed (Inspect) Jan 6, 2025 1:49pm
test-studio ✅ Ready (Inspect) Visit Preview 💬 Add feedback Jan 6, 2025 1:49pm
1 Skipped Deployment
Name Status Preview Comments Updated (UTC)
studio-workshop ⬜️ Ignored (Inspect) Visit Preview Jan 6, 2025 1:49pm

Copy link

socket-security bot commented Jan 3, 2025

New and removed dependencies detected. Learn more about Socket for GitHub ↗︎

Package New capabilities Transitives Size Publisher
npm/react@19.0.0 None 0 0 B

🚮 Removed packages: npm/next@14.2.22, npm/react@19.0.0-rc-f994737d14-20240522

View full report↗︎

Copy link
Contributor

github-actions bot commented Jan 3, 2025

No changes to documentation

Copy link
Contributor

github-actions bot commented Jan 3, 2025

Component Testing Report Updated Jan 6, 2025 1:05 PM (UTC)

✅ All Tests Passed -- expand for details
File Status Duration Passed Skipped Failed
comments/CommentInput.spec.tsx ✅ Passed (Inspect) 1m 8s 15 0 0
formBuilder/ArrayInput.spec.tsx ✅ Passed (Inspect) 12s 3 0 0
formBuilder/inputs/PortableText/Annotations.spec.tsx ✅ Passed (Inspect) 37s 6 0 0
formBuilder/inputs/PortableText/copyPaste/CopyPaste.spec.tsx ✅ Passed (Inspect) 50s 11 7 0
formBuilder/inputs/PortableText/copyPaste/CopyPasteFields.spec.tsx ✅ Passed (Inspect) 0s 0 12 0
formBuilder/inputs/PortableText/Decorators.spec.tsx ✅ Passed (Inspect) 26s 6 0 0
formBuilder/inputs/PortableText/DisableFocusAndUnset.spec.tsx ✅ Passed (Inspect) 14s 3 0 0
formBuilder/inputs/PortableText/DragAndDrop.spec.tsx ✅ Passed (Inspect) 26s 6 0 0
formBuilder/inputs/PortableText/FocusTracking.spec.tsx ✅ Passed (Inspect) 1m 6s 15 0 0
formBuilder/inputs/PortableText/Input.spec.tsx ✅ Passed (Inspect) 1m 27s 21 0 0
formBuilder/inputs/PortableText/ObjectBlock.spec.tsx ✅ Passed (Inspect) 1m 39s 18 0 0
formBuilder/inputs/PortableText/PresenceCursors.spec.tsx ✅ Passed (Inspect) 13s 3 9 0
formBuilder/inputs/PortableText/Styles.spec.tsx ✅ Passed (Inspect) 26s 6 0 0
formBuilder/inputs/PortableText/Toolbar.spec.tsx ✅ Passed (Inspect) 1m 42s 21 0 0
formBuilder/tree-editing/TreeEditing.spec.tsx ✅ Passed (Inspect) 0s 0 3 0
formBuilder/tree-editing/TreeEditingNestedObjects.spec.tsx ✅ Passed (Inspect) 0s 0 3 0

Copy link
Contributor

github-actions bot commented Jan 3, 2025

⚡️ Editor Performance Report

Updated Mon, 06 Jan 2025 13:06:53 GMT

Benchmark reference
latency of sanity@latest
experiment
latency of this branch
Δ (%)
latency difference
article (title) 23.3 efps (43ms) 27.0 efps (37ms) -6ms (-14.0%)
article (body) 53.2 efps (19ms) 51.9 efps (19ms) +0ms (+2.4%)
article (string inside object) 25.3 efps (40ms) 27.8 efps (36ms) -4ms (-8.9%)
article (string inside array) 22.2 efps (45ms) 23.5 efps (43ms) -3ms (-5.6%)
recipe (name) 45.5 efps (22ms) 52.6 efps (19ms) -3ms (-13.6%)
recipe (description) 50.0 efps (20ms) 62.5 efps (16ms) -4ms (-20.0%)
recipe (instructions) 99.9+ efps (6ms) 99.9+ efps (6ms) +0ms (-/-%)
synthetic (title) 17.5 efps (57ms) 18.9 efps (53ms) -4ms (-7.0%)
synthetic (string inside object) 18.7 efps (54ms) 20.0 efps (50ms) -4ms (-6.5%)

efps — editor "frames per second". The number of updates assumed to be possible within a second.

Derived from input latency. efps = 1000 / input_latency

Detailed information

🏠 Reference result

The performance result of sanity@latest

Benchmark latency p75 p90 p99 blocking time test duration
article (title) 43ms 48ms 57ms 532ms 432ms 11.8s
article (body) 19ms 21ms 24ms 153ms 233ms 5.4s
article (string inside object) 40ms 42ms 44ms 137ms 139ms 6.9s
article (string inside array) 45ms 48ms 57ms 181ms 430ms 7.4s
recipe (name) 22ms 23ms 27ms 43ms 0ms 7.0s
recipe (description) 20ms 22ms 24ms 41ms 0ms 4.8s
recipe (instructions) 6ms 7ms 8ms 11ms 0ms 3.1s
synthetic (title) 57ms 60ms 67ms 261ms 1069ms 14.2s
synthetic (string inside object) 54ms 57ms 61ms 758ms 1009ms 9.3s

🧪 Experiment result

The performance result of this branch

Benchmark latency p75 p90 p99 blocking time test duration
article (title) 37ms 41ms 57ms 383ms 733ms 10.3s
article (body) 19ms 21ms 25ms 120ms 66ms 5.3s
article (string inside object) 36ms 39ms 42ms 91ms 168ms 6.5s
article (string inside array) 43ms 44ms 48ms 160ms 194ms 6.9s
recipe (name) 19ms 22ms 25ms 35ms 0ms 6.8s
recipe (description) 16ms 17ms 19ms 29ms 0ms 4.3s
recipe (instructions) 6ms 9ms 9ms 16ms 0ms 3.1s
synthetic (title) 53ms 55ms 58ms 165ms 837ms 12.4s
synthetic (string inside object) 50ms 54ms 62ms 412ms 976ms 8.1s

📚 Glossary

column definitions

  • benchmark — the name of the test, e.g. "article", followed by the label of the field being measured, e.g. "(title)".
  • latency — the time between when a key was pressed and when it was rendered. derived from a set of samples. the median (p50) is shown to show the most common latency.
  • p75 — the 75th percentile of the input latency in the test run. 75% of the sampled inputs in this benchmark were processed faster than this value. this provides insight into the upper range of typical performance.
  • p90 — the 90th percentile of the input latency in the test run. 90% of the sampled inputs were faster than this. this metric helps identify slower interactions that occurred less frequently during the benchmark.
  • p99 — the 99th percentile of the input latency in the test run. only 1% of sampled inputs were slower than this. this represents the worst-case scenarios encountered during the benchmark, useful for identifying potential performance outliers.
  • blocking time — the total time during which the main thread was blocked, preventing user input and UI updates. this metric helps identify performance bottlenecks that may cause the interface to feel unresponsive.
  • test duration — how long the test run took to complete.

@stipsan stipsan force-pushed the test-studio-react-19 branch from 3c46a12 to 90899ff Compare January 3, 2025 18:27
@stipsan stipsan marked this pull request as ready for review January 3, 2025 19:52
@stipsan stipsan requested a review from a team as a code owner January 3, 2025 19:52
@stipsan stipsan requested review from ryanbonial and removed request for a team January 3, 2025 19:52
@stipsan stipsan enabled auto-merge January 3, 2025 19:52
@stipsan stipsan force-pushed the test-studio-react-19 branch from 90899ff to 4bf4355 Compare January 6, 2025 09:52
@stipsan stipsan force-pushed the test-studio-react-19 branch from 4bf4355 to f77be85 Compare January 6, 2025 12:54
@bjoerge
Copy link
Member

bjoerge commented Jan 6, 2025

This is amazing! Very happy to also see the test-next-studio being removed.

I agree with your assessment about using 19 as baseline. Thanks for laying out the pros&cons so clearly!

I think it's super important that we also preserve the ability for devs to easily:

  1. run the studio with react 18, e.g. to investigate bug reports or to quickly verify that a change also works with react 18 (would love to see a follow up PR for pnpm dev:react-18 as you suggests)
  2. To the best of our ability catch accidental usage of react 19 only features/APIs not supported by React 18. I guess this is also partially about developer awareness, so we should make sure everyone knows that we can't start using these features yet and also to look out for usage of react 19 features only in code reviews.
  3. Run our automated tests against both versions (if possible).

@stipsan stipsan added this pull request to the merge queue Jan 6, 2025
Merged via the queue into next with commit 9ed1ff7 Jan 6, 2025
54 of 56 checks passed
@stipsan stipsan deleted the test-studio-react-19 branch January 6, 2025 13:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants