[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 😆
There was a problem hiding this comment.
I'm with Gerard on this one.
There was a problem hiding this comment.
I'm surprised that this would be your preference since the DOM use case would really be best handled with the latest reconciled editor state rather than the pending editor state since the DOM is not necessarily in sync with that yet.
| @@ -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"
| * @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.
I'm with Gerard on this one.
| @@ -22,6 +22,7 @@ import { | |||
| $createNodeSelection, | |||
| $createParagraphNode, | |||
| $createTextNode, | |||
| $getEditor, | |||
There was a problem hiding this comment.
Could I add a small request to add a test, that checks if '$getNearestNodeFromDOMNode' can be called within the new "read" method.
There was a problem hiding this comment.
Sure, it does work, just added tests. Noticed that there's a small inconsistency in that the root node is never set in the _keyToDOMMap. Not sure if is really intentional or not.
It does only run the tests with the latest reconciled state (the current default) because that is really what probably makes the most sense for this use case.
|
It seems there's not yet any consensus as to whether pending: @zurfyx, @ivailop7 I don't have any strong preference, as long as both are possible. I lean slightly towards previous state because it matches what's rendered (and after transforms) which is possibly a more generally useful scenario. I think users can be surprised either way ("read doesn't have my latest writes!" or "the dom doesn't match what I'm reading! shouldn't my transforms prevent the state from looking like this?!"). For my personal use cases I mostly just wanted to be able to get a handle to the editor from a read to get access to information that's independent of the current state (config, theme, builder, etc.). I also don't have any strong preference as to whether we add options to Poking around in the implementation of |
|
I think we'd have to add a different method from Adding a flush-on-read (or flush-on-whatever-read-calls) and having it work nested at the end of an update is probably necessary because command listeners are wrapped in an implicit update but I think a lot of code doesn't really consider that and sets up their own read contexts. I've implemented flush-on-read in the latest commit. |
|
I updated the branch and the tests are passing again (I guess main was broken earlier this week) |
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