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

[Breaking Change][lexical] Bug Fix: Commit updates on editor.setRootElement(null) #7023

Merged

Conversation

etrepum
Copy link
Collaborator

@etrepum etrepum commented Jan 7, 2025

Description

setRootElement(null) now triggers the same sort of reconciliation with a history-merge tag that setRootElement(element) triggered.

Previously, when setRootElement(null) was called with any pending updates, it would put the editor in a situation where editor.getEditorState() would return a writable EditorState. Now the invariant is preserved such that editor.getEditorState() always returns a fully reconciled and read-only EditorState, but to match previous semantics such that the current editorState would synchronously get set from the pendingEditorState, setRootElement now always causes a commit when the root element changes instead of just when it changes to a non-null element.

I think there were likely other subtle bugs due to the previous behavior, since setting _editorState directly from _pendingEditorState also won't run transforms, but it would only be possible to notice if you were inspecting this state while still "headless" since a full reconcile would be performed once a new root element is attached.

Closes #7022

Upgrade Information

This change means that a reconciliation will happen on setRootElement(null). Previously, this call would set the pending editor state to the current editor state which means that update listeners, transforms, etc. would not run in this particular scenario. Now they do.

This should not have any effect on normal code, but it is a breaking change because it can trigger test failures if you are counting the number of times that an update listener is called, or doing something similar to that.

Test plan

Before

If setRootElement(null) was called with any pending updates, it would put the editor in a situation where editor.getEditorState() would return a writable EditorState. Now the invariant is preserved such that editor.getEditorState() always returns a fully reconciled and read-only EditorState, but to match previous semantics setRootElement now always causes a commit when the root element changes.

The problems due to this invariant being violated surfaced after #6894 was released

After

Unit test coverage for this readOnly scenario.

Copy link

vercel bot commented Jan 7, 2025

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 Jan 7, 2025 7:16pm
lexical-playground ✅ Ready (Inspect) Visit Preview 💬 Add feedback Jan 7, 2025 7:16pm

@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 Jan 7, 2025
@etrepum etrepum added extended-tests Run extended e2e tests on a PR and removed CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. labels Jan 7, 2025
@etrepum
Copy link
Collaborator Author

etrepum commented Jan 7, 2025

/cc @potatowagon

Copy link

github-actions bot commented Jan 7, 2025

size-limit report 📦

Path Size
lexical - cjs 31.39 KB (0%)
lexical - esm 31.19 KB (0%)
@lexical/rich-text - cjs 40.47 KB (0%)
@lexical/rich-text - esm 33.12 KB (0%)
@lexical/plain-text - cjs 39.04 KB (0%)
@lexical/plain-text - esm 30.37 KB (0%)
@lexical/react - cjs 42.3 KB (0%)
@lexical/react - esm 34.42 KB (0%)

@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 Jan 7, 2025
@potatowagon
Copy link
Contributor

This should not have any effect on normal code, but it is a breaking change because it can trigger test failures if you are counting the number of times that an update listener is called, or doing something similar to that.

thanks for clarifying the effects of the change

@potatowagon
Copy link
Contributor

would this be considered a minor breaking change? given it should work for most cases. edge case would be an implementation that depends on the number of times an update listener is called

@etrepum
Copy link
Collaborator Author

etrepum commented Jan 9, 2025

Yes, a very minor breaking change. If it breaks anything, the code was either counting calls or doing something that's probably wrong.

@etrepum etrepum added this pull request to the merge queue Jan 9, 2025
Merged via the queue into facebook:main with commit d7abafd Jan 9, 2025
72 of 73 checks passed
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
Development

Successfully merging this pull request may close these issues.

3 participants