[lexical] Feature: Implement Editor.read and EditorState.read with editor argument #6347
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
facebook-github-bot
added
the
CLA Signed
size-limit report 📦
|
etrepum
changed the title
etrepum
requested review from
zurfyx,
fantactuka,
acywatson,
Fetz,
ivailop7,
Sahejkm and
potatowagon
as code owners
There was a problem hiding this comment.
Thank you Bob! Added two opinionated comments, I wonder what the rest think, particularly the folks who were involved in the discussion @fantactuka @ivailop7 @StyleT
| * @param options.pending - Use the pending editorState. Use this only when | ||
| * it is necessary to read the state that has not yet been reconciled (this | ||
| * is the state that you would be working with from editor.update). |
There was a problem hiding this comment.
IMO this behavior is confusing, .read should always grab the pending state just like update. You would not expect an update to be done on top of the reconciler, but rather on top of your previous changes, likewise for
editor.update(() => {
// append paragraph
});
editor.read(() => {
// paragraph should be here
});
There was a problem hiding this comment.
I don't hold any strong opinions on the API, so long as the option exists. I implemented it this way mostly based on the loudest opinions at the time 😆
| @@ -108,8 +112,12 @@ export class EditorState { | |||
| return this._nodeMap.size === 1 && this._selection === null; | |||
| } | |||
|
|
|||
| read<V>(callbackFn: () => V): V { | |||
| return readEditorState(this, callbackFn); | |||
| read<V>(callbackFn: () => V, options?: EditorStateReadOptions): V { | |||
There was a problem hiding this comment.
I'm not a fan of this option like I stated in #6346 (comment)
While the intent is valid, I feel like it can easily lead to pitfalls where the editor is no longer compatible with the EditorState.
For reference, @ivailop7 mentioned a real-use case around DOM (for tables) and this can potentially fail and be hard to debug with this API, whereas editor.read is intuitive and always does what expected.
There was a problem hiding this comment.
I'm totally open to leaving EditorState.read's signature alone and have Editor.read call readEditorState directly. I'll wait until there seems to be some consensus before changing it.
There was a problem hiding this comment.
I only implemented it this way because it seemed to be the stated preference of @StyleT on the call and @fantactuka in #6346
There was a problem hiding this comment.
My point was that we shouldn't have 2 ways of reading the state. But we may sugar coat some API. As mentioned in #6346 (comment) the idea is the following:
editor.readcallsEditorState.readwhile passing correctactiveEditoras an option- We promote
editor.readin the documentation, while keepingEditorState.readfor backward compatibility reasons and for more advanced use cases
This allows to:
a. Achieve API consistency
b. Avoid confusing "regular users" with 2 similar APIs
c. Reduce code duplication and establish relation between related APIs "in code"
Description
Adds an
optionsargument toEditorState.readallowing theactiveEditorto be set.Adds a new
Editor.readmethod as a convenience that can callEditorState.readwith the editor set.Closes #6346