[lexical-react] Feature: React 19 unit tests

[etrepum]

trafficstars

Description

React 19 has several behavior changes to StrictMode that could easily be thought to be bugs in either React or Lexical but at this point it seems that all cases are "simply" user error. This PR takes the following approach to mitigate this potential issue:

• Use the React 17+ automatic JSX runtime (required for React 19, this was merged in #6088)
• Modernize Lexical code, tests, and doc examples to be (hopefully) correct and warning-free with React 18 and 19 (based on a thorough reading of the React 19 Upgrade Guide) - some of this was cleaning up existing tests to be more correct, not necessarily React 19 related but important for validation. No codemods were run, the only large change was to the tests (adding an act shim), the codemod wouldn't have helped us in a React 18 compatible way.
• Change CI workflow to run a full set of the unit and e2e tests in CI with React 19
• New tests to validate our expectations of how React should interact with Lexical (both in and out of StrictMode)
• New documentation covering @lexical/react best practices, and common mistakes that can be made with useEffect that surface under StrictMode (which are usually real bugs in concurrent mode and/or suspense that are hard to reproduce otherwise)
• Workaround for vite config so that the playground builds with React 19

Other inclusions:

• Fix up some overlapping names in package.json files
• Fix all unit test warnings (including silencing ArtificialNode__DO_NOT_USE warning in __DEV__ everywhere), audit @ts-ignore usage and remove unnecessary usage
• Fix a window mutation leak in one of the unit tests (updated queueMicrotask without restoration)
• Updates TypeScript because we were installing two versions (one for lexical-devtools and one elsewhere)
• Moves integration tests behind extended-tests label

NOTE: @lexical/react does claim compatibility with react >=17.x in its peerDependencies but only ^18.2.0 and beta are tested as of this PR. It would probably be a better use of resources to raise the bar to >=18.x (or just leave it as-is until someone tells us it is broken) than to test further backwards, unless there are significant users that are stuck on 17.x for some reason.

Closes: #6044 Closes: #6126 Closes: #6132

Test plan

Before

• The tests had some hard to debug warnings (due to microtask concurrency and in one case just a partially broken test) with any version of React
• We had no automated way of confirming that everything works with React 19
• Users would commonly run into tricky StrictMode issues that typically required expert help to diagnose

After

• All tests pass without warning in React 18 and 19
• CI confirm that everything works with React 18 and React 19 (technically not 19, but whichever react and react-dom versions are tagged beta at the time of CI)
• Users will probably have the same issues, but we'll have some documentation to point them to

Follow-up Considerations

We could help mitigate more user issues by:

• More aggressively detecting common pitfalls with __DEV__ warnings or invariants (e.g. write a hook that gets eliminated in prod that warns if a particular argument does not have a stable identity)
• Adding lint rules to @lexical/eslint. A class of lint rule that would be useful in this scenario would be detecting functions without stable identity (useCallback, useMemo, useRef, etc. or being defined out of component scope) that are passed as component props or hook arguments that would be problematic like CollaborationPlugin providerFactory or useLexicalSubscription

[vercel[bot]]

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

Name	Status	Preview	Comments	Updated (UTC)
lexical	✅ Ready (Inspect)	Visit Preview	💬 Add feedback	May 21, 2024 9:45pm
lexical-playground	✅ Ready (Inspect)	Visit Preview	💬 Add feedback	May 21, 2024 9:45pm

[etrepum]

This shouldn't be merged until after #6080 as I will need to revisit the workflow additions

[StyleT]

Hi! Looks awesome. Can you pls rebase it and I'll take a look asap.

[etrepum]

@StyleT ready to go, you will need to add the extended-tests label if you want the full e2e suite to run

[etrepum]

The test failure looks like a github fault

https://github.com/facebook/lexical/actions/runs/9106181327/job/25032956714?pr=6048

Download action repository 'actions/checkout@v4' (SHA:0ad4b8fadaa221de15dcec353f45205ec38ea70b)
Warning: Failed to download action 'https://api.github.com/repos/actions/checkout/tarball/0ad4b8fadaa221de15dcec353f45205ec38ea70b'. Error: nodename nor servname provided, or not known (api.github.com:443)
Warning: Back off 12.403 seconds before retry.
Warning: Failed to download action 'https://api.github.com/repos/actions/checkout/tarball/0ad4b8fadaa221de15dcec[35](https://github.com/facebook/lexical/actions/runs/9106181327/job/25032956714?pr=6048#step:1:39)3f45205ec38ea70b'. Error: nodename nor servname provided, or not known (api.github.com:443)
Warning: Back off 15.559 seconds before retry.
Error: nodename nor servname provided, or not known (api.github.com:443)

[etrepum]

Closing and re-opening to kick off another test run

[etrepum]

Doesn't look like that fired off another extended test run

[etrepum]

Hard to tell which test hung but I'll take a look, I suspect restarting the test suite would pass though because they passed before.

[necolas]

The react-beta tests are hanging at the moment. You might want to iterate on that test without running the extended test suite any more times. I'll remove the label for now

[etrepum]

Removing the label wouldn't really help me confirm when it's fixed it unless I move it out of the extended test suite, and then it would get run for all builds if I didn't do another commit after approval (dismissing the approval) to move the workflow back.

[etrepum]

I think something must've changed in the react beta tag that's throwing off rollup's heuristics which causes an issue with the playground build. Taking longer to resolve than I expected.

[etrepum]

So far all I know is that there seems to be some sort of issue with the latest build of react beta and the heuristics that @rollup/plugin-commonjs uses to determine the named exports (another fun fork module day). I'll have to look into it more tomorrow.

RollupError: src/ui/FlashMessage.tsx (12:8): "createPortal" is not exported by "../../node_modules/react-dom/index.js", imported by "src/ui/FlashMessage.tsx".
file: /Users/bob/src/lexical/packages/lexical-playground/src/ui/FlashMessage.tsx:12:8
10: 
11: import {ReactNode} from 'react';
12: import {createPortal} from 'react-dom';
            ^

[necolas]

Yes move it out of extended, get it passing, move it back into extended, get approval. This is also why it shouldn't be running on every PR IMO

[etrepum]

I've disabled that one new e2e test suite (https://github.com/facebook/lexical/pull/6048/commits/774a2979da8969ec064d11f336ac9f35e68adfba), since it is not a regression and this PR includes other important fixes that should be released sooner than later. Every other workflow was passing before this commit, so up to the reviewer to decide to add extended-tests back before merging. The follow-up is #6126

[etrepum]

Yes move it out of extended, get it passing, move it back into extended, get approval. This is also why it shouldn't be running on every PR IMO

In this particular case it was passing, until it wasn't. I think something changed upstream during that time. Anyway, it does not need to be resolved in this PR because it is not a regression, so it is disabled and all tests that run should be passing now.

[etrepum]

Giving it another go since I have it all passing locally. Hopefully CI behaves the same way.

[etrepum]

Test run is green, removing the workflow hack. https://github.com/facebook/lexical/actions/runs/9131368689/job/25110422301?pr=6048

[etrepum]

Merged with main and waiting for review.

[etrepum]

Up to date with main, waiting for review.

[github-actions[bot]]

size-limit report 📦

Path	Size	Loading time (3g)	Running time (snapdragon)	Total time
packages/lexical/dist/Lexical.js	23.93 KB (0%)	479 ms (0%)	425 ms (-13.86% 🔽)	903 ms
packages/lexical-rich-text/dist/LexicalRichText.js	34.53 KB (0%)	691 ms (0%)	1.1 s (+1.26% 🔺)	1.8 s
packages/lexical-plain-text/dist/LexicalPlainText.js	34.59 KB (0%)	692 ms (0%)	1.4 s (+61.77% 🔺)	2.1 s

[etrepum]

Since the last time it was reviewed and ready to merge (but was blocked due to the jsx runtime regression & the react 19 beta rollup regression that has a workaround now), not much has changed:

• Typescript upgrade - we had two versions installed because lexical-devtools was requesting a version that wasn't satisfied by other constraints
• A better react override strategy - this also moves the integration tests to extended-tests since we should not need to run them constantly
• A fix to the react beta e2e test - something changed in React 19 beta from when I started to when we got this back to the approval stage last week and I had to change the rollup config accordingly
• A more robust way to silence the artificial node warnings

Commits that are here that cancel out:

• Adding then removing the react beta e2e tests from the core suite to validate that everything still works (no net change)
• A fix for the jsx build size regression - this was cherry-picked and merged to main so its contents are not relevant anymore (no net change)
• Merge commits without any manual conflict resolution.

[etrepum]

I would really like to see this included before the next release, it fixes all of the noise in the tests (and in __DEV__). It's annoying to work on the tests when they are spewing false positive to the console. Plus it makes everything React 19 ready, of course.

[etrepum]

All tests are currently passing, the failure is the label-on-approval workflow issue being debugged in #6153

[ivailop7]

Sure, I'm OK with it, but someone from Meta needs to confirm they are good with this one.