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

[WIP][lexical-react] Bug Fix: React 19 StrictMode correctness #6041

Closed
wants to merge 3 commits into from

Conversation

etrepum
Copy link
Collaborator

@etrepum etrepum commented May 6, 2024

Description

React 19's StrictMode re-uses the results of useMemo between renders (notes). We have code in the composers that uses useMemo to create LexicalEditor which are a mutable state nightmare if shared, causing all sorts of hard to understand bugs and makes us look pretty bad in dev modes everywhere. useState(…)[0] should not have this issue.

We can't fix the fact that you can of course make similar mistakes in your own components. We could also add documentation to the FAQ or somewhere else that shows best practices. I'm not sure exactly what practice we would recommend if you want to do some sort of editor mutation that the cleanup function doesn't take care of (e.g. set up some initial editor state with a plugin).

Closes: #6040

Test plan

Before

See #6040 for a full example

After

Unit tests pass. Not sure what the best way to run the test suite with React 19 in CI would be?

… creating LexicalEditor for React 19 StrictMode correctness
Copy link

vercel bot commented May 6, 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 6, 2024 10:40pm
lexical-playground ✅ Ready (Inspect) Visit Preview 💬 Add feedback May 6, 2024 10:40pm

@facebook-github-bot facebook-github-bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label May 6, 2024
@etrepum
Copy link
Collaborator Author

etrepum commented May 7, 2024

After thinking about this some more I think the approach we have is probably correct enough, but we have some things to address regarding React 19:

  • CI should run a separate suite of tests against react@beta
  • Track down deprecated APIs and replace them with React 17,18,19 compatible approaches
  • Write some documentation about all the ways you can muck things up when effects are not used correctly, especially in StrictMode (e.g. trying to exfiltrate data from the editorState callback) - promoting functionality like useLexicalSubscription instead of error-prone DIY
  • Have some unit tests that run in StrictMode to try and catch potential issues

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.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants