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.

Sign up for GitHub

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

Jump to bottom

[lexical] Feature: Implement Editor.read and EditorState.read with editor argument #6347

Merged
merged 10 commits into from
Jul 14, 2024
Merged

[lexical] Feature: Implement Editor.read and EditorState.read with editor argument #6347

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
cb2b260
[WIP] Implement Editor.read and EditorState.read with editor argument
etrepum Jun 27, 2024
88ac5fc
Flow types for Editor.read and EditorState.read
etrepum Jun 28, 2024
876e3a0
Tests for Editor.read and EditorState.read
etrepum Jun 28, 2024
bb8c31e
Nested read tests and update docs
etrepum Jun 28, 2024
353e755
reformat note for docusaurus
etrepum Jun 28, 2024
004ad87
Add tests for $getNearestNodeFromDOMNode
etrepum Jun 30, 2024
71be817
Remove EditorReadOptions and first commit pending updates
etrepum Jul 1, 2024
7996209
Merge branch 'main' into editor-read
etrepum Jul 1, 2024
2a8cb98
Merge branch 'main' into editor-read
etrepum Jul 3, 2024
a744382
Merge branch 'main' into editor-read
etrepum Jul 12, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
23 changes: 17 additions & 6 deletions README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -153,11 +153,11 @@ Editor States are also fully serializable to JSON and can easily be serialized b
### Reading and Updating Editor State

When you want to read and/or update the Lexical node tree, you must do it via `editor.update(() => {...})`. You may also do
read-only operations with the editor state via `editor.getEditorState().read(() => {...})`. The closure passed to the update or read
call is important, and must be synchronous. It's the only place where you have full "lexical" context of the active editor state,
and providing you with access to the Editor State's node tree. We promote using the convention of using `$` prefixed functions
(such as `$getRoot()`) to convey that these functions must be called in this context. Attempting to use them outside of a read
or update will trigger a runtime error.
read-only operations with the editor state via `editor.getEditorState().read(() => {...})` or `editor.read(() => {...})`.
The closure passed to the update or read call is important, and must be synchronous. It's the only place where you have full
"lexical" context of the active editor state, and providing you with access to the Editor State's node tree. We promote using
the convention of using `$` prefixed functions (such as `$getRoot()`) to convey that these functions must be called in this
context. Attempting to use them outside of a read or update will trigger a runtime error.

For those familiar with React Hooks, you can think of these $functions as having similar functionality:
| *Feature* | React Hooks | Lexical $functions |
Expand All @@ -170,7 +170,7 @@ For those familiar with React Hooks, you can think of these $functions as having

Node Transforms and Command Listeners are called with an implicit `editor.update(() => {...})` context.

It is permitted to do nested updates within reads and updates, but an update may not be nested in a read.
It is permitted to do nested reads and updates, but an update should not be nested in a read.
For example, `editor.update(() => editor.update(() => {...}))` is allowed.

All Lexical Nodes are dependent on the associated Editor State. With few exceptions, you should only call methods
Expand All @@ -186,6 +186,17 @@ first call `node.getWritable()`, which will create a writable clone of a frozen
mean that any existing references (such as local variables) would refer to a stale version of the node, but
having Lexical Nodes always refer to the editor state allows for a simpler and less error-prone data model.

:::tip

`editor.getEditorState().read()` and `editor.read()` will use the latest
reconciled `EditorState` (after any node transforms, DOM reconciliation,
etc. have already run). Any pending `editor.update` calls that were not
scheduled with `discrete: true` will not yet be visible unless you call
`editor.read(() => { /* callback */ }, { pending: true })`. When you are
in an `editor.update`, you will always see the pending state.

:::

### DOM Reconciler

Lexical has its own DOM reconciler that takes a set of Editor States (always the "current" and the "pending") and applies a "diff"
Expand Down
23 changes: 17 additions & 6 deletions packages/lexical-website/docs/intro.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -62,11 +62,11 @@ Editor States are also fully serializable to JSON and can easily be serialized b
### Reading and Updating Editor State

When you want to read and/or update the Lexical node tree, you must do it via `editor.update(() => {...})`. You may also do
read-only operations with the editor state via `editor.getEditorState().read(() => {...})`. The closure passed to the update or read
call is important, and must be synchronous. It's the only place where you have full "lexical" context of the active editor state,
and providing you with access to the Editor State's node tree. We promote using the convention of using `$` prefixed functions
(such as `$getRoot()`) to convey that these functions must be called in this context. Attempting to use them outside of a read
or update will trigger a runtime error.
read-only operations with the editor state via `editor.getEditorState().read(() => {...})` or `editor.read(() => {...})`.
The closure passed to the update or read call is important, and must be synchronous. It's the only place where you have full
"lexical" context of the active editor state, and providing you with access to the Editor State's node tree. We promote using
the convention of using `$` prefixed functions (such as `$getRoot()`) to convey that these functions must be called in this
context. Attempting to use them outside of a read or update will trigger a runtime error.

For those familiar with React Hooks, you can think of these $functions as having similar functionality:
| *Feature* | React Hooks | Lexical $functions |
Expand All @@ -79,7 +79,7 @@ For those familiar with React Hooks, you can think of these $functions as having

Node Transforms and Command Listeners are called with an implicit `editor.update(() => {...})` context.

It is permitted to do nest updates within reads and updates, but an update may not be nested in a read.
It is permitted to do nested reads and updates, but an update should not be nested in a read.
For example, `editor.update(() => editor.update(() => {...}))` is allowed.

All Lexical Nodes are dependent on the associated Editor State. With few exceptions, you should only call methods
Expand All @@ -95,6 +95,17 @@ first call `node.getWritable()`, which will create a writable clone of a frozen
mean that any existing references (such as local variables) would refer to a stale version of the node, but
having Lexical Nodes always refer to the editor state allows for a simpler and less error-prone data model.

:::tip

`editor.getEditorState().read()` and `editor.read()` will use the latest
reconciled `EditorState` (after any node transforms, DOM reconciliation,
etc. have already run). Any pending `editor.update` calls that were not
scheduled with `discrete: true` will not yet be visible unless you call
`editor.read(() => { /* callback */ }, { pending: true })`. When you are
in an `editor.update`, you will always see the pending state.

:::

### DOM Reconciler

Lexical has its own DOM reconciler that takes a set of Editor States (always the "current" and the "pending") and applies a "diff"
Expand Down
9 changes: 8 additions & 1 deletion packages/lexical/flow/Lexical.js.flow
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -198,13 +198,17 @@ declare export class LexicalEditor {
maybeStringifiedEditorState: string | SerializedEditorState,
updateFn?: () => void,
): EditorState;
read<V>(callbackFn: () => V, options?: EditorReadOptions): V;
update(updateFn: () => void, options?: EditorUpdateOptions): boolean;
focus(callbackFn?: () => void, options?: EditorFocusOptions): void;
blur(): void;
isEditable(): boolean;
setEditable(editable: boolean): void;
toJSON(): SerializedEditor;
}
type EditorReadOptions = {
pending?: boolean,
};
type EditorUpdateOptions = {
onUpdate?: () => void,
tag?: string,
Expand Down Expand Up @@ -324,10 +328,13 @@ export interface EditorState {
_readOnly: boolean;
constructor(nodeMap: NodeMap, selection?: BaseSelection | null): void;
isEmpty(): boolean;
read<V>(callbackFn: () => V): V;
read<V>(callbackFn: () => V, options?: EditorStateReadOptions): V;
toJSON(): SerializedEditorState;
clone(selection?: BaseSelection | null): EditorState;
}
type EditorStateReadOptions = {
editor?: LexicalEditor | null;
}

/**
* LexicalNode
Expand Down
24 changes: 23 additions & 1 deletion packages/lexical/src/LexicalEditor.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -82,6 +82,10 @@ export type EditorUpdateOptions = {
discrete?: true;
};

export interface EditorReadOptions {
pending?: boolean;
}

export type EditorSetOptions = {
tag?: string;
};
Expand Down Expand Up @@ -1097,7 +1101,7 @@ export class LexicalEditor {
/**
* Parses a SerializedEditorState (usually produced by {@link EditorState.toJSON}) and returns
* and EditorState object that can be, for example, passed to {@link LexicalEditor.setEditorState}. Typically,
* deserliazation from JSON stored in a database uses this method.
* deserialization from JSON stored in a database uses this method.
* @param maybeStringifiedEditorState
* @param updateFn
* @returns
Expand All @@ -1113,6 +1117,24 @@ export class LexicalEditor {
return parseEditorState(serializedEditorState, this, updateFn);
}

/**
* Executes a read of the editor's state, with the
* editor context available (useful for exporting and read-only DOM
* operations). Much like update, but prevents any mutation of the
* editor's state.
* @param callbackFn - A function that has access to read-only editor state.
* @param options - A bag of options to control the behavior of the read.
* @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).
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
});

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 😆

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm with Gerard on this one.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

*/
read<T>(callbackFn: () => T, options?: EditorReadOptions): T {
const editorState =
(options && options.pending && this._pendingEditorState) ||
this.getEditorState();
return editorState.read(callbackFn, {editor: this});
}

/**
* Executes an update to the editor state. The updateFn callback is the ONLY place
* where Lexical editor state can be safely mutated.
Expand Down
14 changes: 11 additions & 3 deletions packages/lexical/src/LexicalEditorState.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -91,6 +91,10 @@ function exportNodeToJSON<SerializedNode extends SerializedLexicalNode>(
return serializedNode;
}

export interface EditorStateReadOptions {
editor?: LexicalEditor | null;
}

export class EditorState {
_nodeMap: NodeMap;
_selection: null | BaseSelection;
Expand All @@ -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 {
Copy link
Member

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.

Copy link
Collaborator Author

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 have Editor.read call readEditorState directly. I'll wait until there seems to be some consensus before changing it.

Copy link
Collaborator Author

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

Copy link
Contributor

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:

  1. editor.read calls EditorState.read while passing correct activeEditor as an option
  2. We promote editor.read in the documentation, while keeping EditorState.read for 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"

return readEditorState(
(options && options.editor) || null,
this,
callbackFn,
);
}

clone(selection?: null | BaseSelection): EditorState {
Expand All @@ -122,7 +130,7 @@ export class EditorState {
return editorState;
}
toJSON(): SerializedEditorState {
return readEditorState(this, () => ({
return readEditorState(null, this, () => ({
root: exportNodeToJSON($getRoot()),
}));
}
Expand Down
3 changes: 2 additions & 1 deletion packages/lexical/src/LexicalUpdates.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -397,6 +397,7 @@ export function parseEditorState(
// function here

export function readEditorState<V>(
editor: LexicalEditor | null,
editorState: EditorState,
callbackFn: () => V,
): V {
Expand All @@ -406,7 +407,7 @@ export function readEditorState<V>(

activeEditorState = editorState;
isReadOnlyMode = true;
activeEditor = null;
activeEditor = editor;

try {
return callbackFn();
Expand Down
73 changes: 73 additions & 0 deletions packages/lexical/src/__tests__/unit/LexicalEditor.test.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@ import {
$createNodeSelection,
$createParagraphNode,
$createTextNode,
$getEditor,
Copy link
Collaborator

Choose a reason for hiding this comment

The 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.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The 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 _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.

$getNodeByKey,
$getRoot,
$isTextNode,
Expand Down Expand Up @@ -133,6 +134,78 @@ describe('LexicalEditor tests', () => {
return Promise.resolve().then();
}

describe('read()', () => {
it('Can read the editor state', async () => {
init();
expect(editor.read(() => $getRoot().getTextContent())).toEqual('');
expect(editor.read(() => $getEditor())).toBe(editor);
editor.update(() => {
const root = $getRoot();
const paragraph = $createParagraphNode();
const text = $createTextNode('This works!');
root.append(paragraph);
paragraph.append(text);
});
expect(editor.read(() => $getRoot().getTextContent())).toEqual('');
expect(editor.read(() => $getRoot().getTextContent(), {})).toEqual('');
expect(
editor.read(() => $getRoot().getTextContent(), {pending: false}),
).toEqual('');
expect(
editor.read(() => $getRoot().getTextContent(), {pending: true}),
).toEqual('This works!');
await Promise.resolve().then();
editor.read(() => {
$getRoot();
});
expect(editor.read(() => $getRoot().getTextContent())).toEqual(
'This works!',
);
expect(
editor.read(() => $getRoot().getTextContent(), {pending: true}),
).toEqual('This works!');
});

it('Can be nested in an update or read', async () => {
init();
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('');
});
editor.read(
() => {
expect($getRoot().getTextContent()).toBe('This works!');
},
{pending: true},
);
// This works, although it is discouraged in the documentation.
editor.read(() => {
editor.update(() => {
expect($getRoot().getTextContent()).toBe('This works!');
});
expect($getRoot().getTextContent()).toBe('');
editor.read(
() => {
expect($getRoot().getTextContent()).toBe('This works!');
},
{pending: true},
);
});
});
await Promise.resolve().then();
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');

Expand Down
7 changes: 7 additions & 0 deletions packages/lexical/src/__tests__/unit/LexicalEditorState.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@
import {
$createParagraphNode,
$createTextNode,
$getEditor,
$getRoot,
ParagraphNode,
TextNode,
Expand Down Expand Up @@ -89,6 +90,12 @@ describe('LexicalEditorState tests', () => {
__text: 'foo',
__type: 'text',
});
expect(() => editor.getEditorState().read(() => $getEditor())).toThrow(
/Unable to find an active editor/,
);
expect(
editor.getEditorState().read(() => $getEditor(), {editor: editor}),
).toBe(editor);
});

test('toJSON()', async () => {
Expand Down
7 changes: 6 additions & 1 deletion packages/lexical/src/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@ export type {
CreateEditorArgs,
EditableListener,
EditorConfig,
EditorReadOptions,
EditorSetOptions,
EditorThemeClasses,
EditorThemeClassName,
Expand All @@ -30,7 +31,11 @@ export type {
Spread,
Transform,
} from './LexicalEditor';
export type {EditorState, SerializedEditorState} from './LexicalEditorState';
export type {
EditorState,
EditorStateReadOptions,
SerializedEditorState,
} from './LexicalEditorState';
export type {
DOMChildConversion,
DOMConversion,
Expand Down
Loading