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

[lexical-react] Feature: React 19 unit tests #6048

Merged
merged 34 commits into from
May 22, 2024

Conversation

etrepum
Copy link
Collaborator

@etrepum etrepum commented May 7, 2024

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 [lexical-react] Breaking change: Deprecate public default exports #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
Closes: #5911

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

Copy link

vercel bot commented May 7, 2024

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

.github/workflows/test.yml Outdated Show resolved Hide resolved
@@ -125,7 +125,7 @@ describe('LexicalNormalization tests', () => {

const selection = paragraph.select();
getAnchor(selection).set(text1.__key, 0, 'text');
getFocus(selection).set(text2.__key, 2, 'text');
getFocus(selection).set(text2.__key, 1, 'text');
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This fixes a bug in the test, the values here were wrong (text2 is only 1 character long, so an offset of 2 does not make sense and jest-dom correctly throws an error later, but that error gets turned into a warning as a firefox-mitigation)

@etrepum
Copy link
Collaborator Author

etrepum commented May 17, 2024

Merged with main and waiting for review.

@@ -46,20 +46,22 @@ describe('LexicalUtils tests', () => {

test('scheduleMicroTask(): promise', async () => {
jest.resetModules();
// @ts-ignore
window.queueMicrotask = undefined;
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This was never unset, so I rewrote this test

@etrepum
Copy link
Collaborator Author

etrepum commented May 21, 2024

Up to date with main, waiting for review.

Copy link

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

Copy link
Collaborator

@ivailop7 ivailop7 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Because you've been rebasing this one constantly, I want to unblock you, so I'll approve it, given the tests pass. Ideally, a second pair of eyes here would be nice. @acywatson @zurfyx @StyleT @Sahejkm could someone help out here.

@ivailop7 ivailop7 added the extended-tests Run extended e2e tests on a PR label May 21, 2024
@etrepum
Copy link
Collaborator Author

etrepum commented May 21, 2024

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:

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
Copy link
Collaborator Author

etrepum commented May 21, 2024

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
Copy link
Collaborator Author

etrepum commented May 21, 2024

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

@ivailop7
Copy link
Collaborator

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

@acywatson acywatson added this pull request to the merge queue May 22, 2024
Merged via the queue into facebook:main with commit 53f6f1d May 22, 2024
44 of 45 checks passed
sqs added a commit to sourcegraph/cody that referenced this pull request May 27, 2024
Note: The JS console has warnings about `ArtificialNode__DO_NOT_USE`. You can ignore these. They will be fixed in Lexical's next release; see facebook/lexical#6048.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. extended-tests Run extended e2e tests on a PR
Projects
None yet
8 participants