-
Notifications
You must be signed in to change notification settings - Fork 1.7k
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
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
@@ -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'); |
There was a problem hiding this comment.
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)
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; |
There was a problem hiding this comment.
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
Up to date with main, waiting for review. |
size-limit report 📦
|
There was a problem hiding this 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.
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:
|
I would really like to see this included before the next release, it fixes all of the noise in the tests (and in |
All tests are currently passing, the failure is the label-on-approval workflow issue being debugged in #6153 |
Sure, I'm OK with it, but someone from Meta needs to confirm they are good with this one. |
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.
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:
Other inclusions:
ArtificialNode__DO_NOT_USE
warning in__DEV__
everywhere), audit@ts-ignore
usage and remove unnecessary usagequeueMicrotask
without restoration)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
After
Follow-up Considerations
We could help mitigate more user issues by:
__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)