-
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] Feature: Implement Editor.read and EditorState.read with editor argument #6347
Changes from all commits
cb2b260
88ac5fc
876e3a0
bb8c31e
353e755
004ad87
71be817
7996209
2a8cb98
a744382
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -22,8 +22,11 @@ import { | |
$createNodeSelection, | ||
$createParagraphNode, | ||
$createTextNode, | ||
$getEditor, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Sure, it does work, just added tests. Noticed that there's a small inconsistency in that the root node is never set in the 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. |
||
$getNearestNodeFromDOMNode, | ||
$getNodeByKey, | ||
$getRoot, | ||
$isParagraphNode, | ||
$isTextNode, | ||
$parseSerializedNode, | ||
$setCompositionKey, | ||
|
@@ -113,7 +116,7 @@ describe('LexicalEditor tests', () => { | |
|
||
let editor: LexicalEditor; | ||
|
||
function init(onError?: () => void) { | ||
function init(onError?: (error: Error) => void) { | ||
const ref = createRef<HTMLDivElement>(); | ||
|
||
function TestBase() { | ||
|
@@ -133,6 +136,130 @@ describe('LexicalEditor tests', () => { | |
return Promise.resolve().then(); | ||
} | ||
|
||
describe('read()', () => { | ||
it('Can read the editor state', async () => { | ||
init(function onError(err) { | ||
throw err; | ||
}); | ||
expect(editor.read(() => $getRoot().getTextContent())).toEqual(''); | ||
expect(editor.read(() => $getEditor())).toBe(editor); | ||
const onUpdate = jest.fn(); | ||
editor.update( | ||
() => { | ||
const root = $getRoot(); | ||
const paragraph = $createParagraphNode(); | ||
const text = $createTextNode('This works!'); | ||
root.append(paragraph); | ||
paragraph.append(text); | ||
}, | ||
{onUpdate}, | ||
); | ||
expect(onUpdate).toHaveBeenCalledTimes(0); | ||
// This read will flush pending updates | ||
expect(editor.read(() => $getRoot().getTextContent())).toEqual( | ||
'This works!', | ||
); | ||
expect(onUpdate).toHaveBeenCalledTimes(1); | ||
// Check to make sure there is not an unexpected reconciliation | ||
await Promise.resolve().then(); | ||
expect(onUpdate).toHaveBeenCalledTimes(1); | ||
editor.read(() => { | ||
const rootElement = editor.getRootElement(); | ||
expect(rootElement).toBeDefined(); | ||
// The root never works for this call | ||
expect($getNearestNodeFromDOMNode(rootElement!)).toBe(null); | ||
const paragraphDom = rootElement!.querySelector('p'); | ||
expect(paragraphDom).toBeDefined(); | ||
expect( | ||
$isParagraphNode($getNearestNodeFromDOMNode(paragraphDom!)), | ||
).toBe(true); | ||
expect( | ||
$getNearestNodeFromDOMNode(paragraphDom!)!.getTextContent(), | ||
).toBe('This works!'); | ||
const textDom = paragraphDom!.querySelector('span'); | ||
expect(textDom).toBeDefined(); | ||
expect($isTextNode($getNearestNodeFromDOMNode(textDom!))).toBe(true); | ||
expect($getNearestNodeFromDOMNode(textDom!)!.getTextContent()).toBe( | ||
'This works!', | ||
); | ||
expect( | ||
$getNearestNodeFromDOMNode(textDom!.firstChild!)!.getTextContent(), | ||
).toBe('This works!'); | ||
}); | ||
expect(onUpdate).toHaveBeenCalledTimes(1); | ||
}); | ||
it('runs transforms the editor state', async () => { | ||
init(function onError(err) { | ||
throw err; | ||
}); | ||
expect(editor.read(() => $getRoot().getTextContent())).toEqual(''); | ||
expect(editor.read(() => $getEditor())).toBe(editor); | ||
editor.registerNodeTransform(TextNode, (node) => { | ||
if (node.getTextContent() === 'This works!') { | ||
node.replace($createTextNode('Transforms work!')); | ||
} | ||
}); | ||
const onUpdate = jest.fn(); | ||
editor.update( | ||
() => { | ||
const root = $getRoot(); | ||
const paragraph = $createParagraphNode(); | ||
const text = $createTextNode('This works!'); | ||
root.append(paragraph); | ||
paragraph.append(text); | ||
}, | ||
{onUpdate}, | ||
); | ||
expect(onUpdate).toHaveBeenCalledTimes(0); | ||
// This read will flush pending updates | ||
expect(editor.read(() => $getRoot().getTextContent())).toEqual( | ||
'Transforms work!', | ||
); | ||
expect(editor.getRootElement()!.textContent).toEqual('Transforms work!'); | ||
expect(onUpdate).toHaveBeenCalledTimes(1); | ||
// Check to make sure there is not an unexpected reconciliation | ||
await Promise.resolve().then(); | ||
expect(onUpdate).toHaveBeenCalledTimes(1); | ||
expect(editor.read(() => $getRoot().getTextContent())).toEqual( | ||
'Transforms work!', | ||
); | ||
}); | ||
it('can be nested in an update or read', async () => { | ||
init(function onError(err) { | ||
throw err; | ||
}); | ||
editor.update(() => { | ||
const root = $getRoot(); | ||
const paragraph = $createParagraphNode(); | ||
const text = $createTextNode('This works!'); | ||
root.append(paragraph); | ||
paragraph.append(text); | ||
editor.read(() => { | ||
expect($getRoot().getTextContent()).toBe('This works!'); | ||
}); | ||
editor.read(() => { | ||
// Nesting update in read works, although it is discouraged in the documentation. | ||
editor.update(() => { | ||
expect($getRoot().getTextContent()).toBe('This works!'); | ||
}); | ||
}); | ||
// Updating after a nested read will fail as it has already been committed | ||
expect(() => { | ||
root.append( | ||
$createParagraphNode().append( | ||
$createTextNode('update-read-update'), | ||
), | ||
); | ||
}).toThrow(); | ||
}); | ||
editor.read(() => { | ||
editor.read(() => { | ||
expect($getRoot().getTextContent()).toBe('This works!'); | ||
}); | ||
}); | ||
}); | ||
}); | ||
|
||
it('Should create an editor with an initial editor state', async () => { | ||
const rootElement = document.createElement('div'); | ||
|
||
|
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm totally open to leaving
EditorState.read
's signature alone and haveEditor.read
callreadEditorState
directly. I'll wait until there seems to be some consensus before changing it.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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.read
callsEditorState.read
while passing correctactiveEditor
as an optioneditor.read
in the documentation, while keepingEditorState.read
for backward compatibility reasons and for more advanced use casesThis 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"