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

[lexical] Feature: add a generic state property to all nodes #7117

Open
wants to merge 20 commits into
base: main
Choose a base branch
from
+276 −5
Open

[lexical] Feature: add a generic state property to all nodes #7117

Show file tree
Hide file tree
Changes from 11 commits
Commits
Show all changes
20 commits
Select commit Hold shift + click to select a range
a6288f9
state to lexicalNode
GermanJablo Dec 19, 2024
a90a719
Squashed commit of the following:
GermanJablo Jan 30, 2025
448e2af
first version with default value
GermanJablo Jan 30, 2025
bf70110
make state json serializable
GermanJablo Jan 30, 2025
6ebd4e5
add import and export json tests
GermanJablo Jan 30, 2025
e46d735
getState returns immutable types
GermanJablo Jan 31, 2025
e404d32
fix tests
GermanJablo Jan 31, 2025
c1cfce4
Merge remote-tracking branch 'origin/main' into state
GermanJablo Jan 31, 2025
6d00c64
add docs
GermanJablo Jan 31, 2025
e0c5945
remove readonly property
GermanJablo Jan 31, 2025
7728780
add paragraph in docs about json serializable values
GermanJablo Jan 31, 2025
d807d4f
better example in docs. getState does not necessarily return undefined.
GermanJablo Jan 31, 2025
90a032b
use $createTextNode()
GermanJablo Jan 31, 2025
c663540
fix type
GermanJablo Jan 31, 2025
113a4f1
create a new object in setState
GermanJablo Jan 31, 2025
91148f3
fix unit test
GermanJablo Jan 31, 2025
47435b0
Update packages/lexical/src/LexicalNode.ts
GermanJablo Jan 31, 2025
fb7e164
Update packages/lexical/src/LexicalNode.ts
GermanJablo Jan 31, 2025
eab152e
Update packages/lexical/src/LexicalNode.ts
GermanJablo Jan 31, 2025
d2b4f21
Update packages/lexical/src/LexicalNode.ts
GermanJablo Jan 31, 2025
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
7 changes: 4 additions & 3 deletions .eslintrc.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -88,7 +88,10 @@ module.exports = {
],
'@typescript-eslint/ban-ts-comment': OFF,
'@typescript-eslint/no-this-alias': OFF,
'@typescript-eslint/no-unused-vars': [ERROR, {args: 'none'}],
'@typescript-eslint/no-unused-vars': [
ERROR,
{args: 'none', argsIgnorePattern: '^_', varsIgnorePattern: '^_'},
],
Comment on lines +91 to +94
Copy link
Contributor Author

Choose a reason for hiding this comment

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

If I may, I would like to propose this form of opt-out which has legitimate uses. For example, in LexicalNode.test.ts when I define type variables just for the purpose of testing the types.
Also, while developing a WIP it's convenient to be able to mute warnings without having to write comments everywhere.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I prefer this setting also, but maybe there’s some meta style guide reason for not having this? /cc @zurfyx

'header/header': [2, 'scripts/www/headerTemplate.js'],
},
},
Expand Down Expand Up @@ -226,8 +229,6 @@ module.exports = {

'no-unused-expressions': ERROR,

'no-unused-vars': [ERROR, {args: 'none'}],

'no-use-before-define': OFF,

// Flow fails with with non-string literal keys
Expand Down
3 changes: 3 additions & 0 deletions packages/lexical-table/src/__tests__/unit/LexicalTableSelection.test.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -134,6 +134,7 @@ describe('table selection', () => {
__parent: null,
__prev: null,
__size: 1,
__state: {},
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Perhaps I could have used Object.defineProperty to avoid having to modify the tests, but I preferred to do this because it turned out that there were few tests affected and the changes were trivial.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I think this will probably be necessary to implement for correct yjs behavior

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I already followed your suggestion to copy the state object in setState, so this suggestion is no longer necessary?
On the other hand, do you know what a unit test for yjs could look like?

Copy link
Collaborator

Choose a reason for hiding this comment

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

I am not familiar with the intricacies of testing lexical-yjs off-hand, I just know its implementation is very problematic because it manipulates node properties directly rather than going through the constructor and well defined serializations. I'd find another node that uses a non-primitive property (something to do with comments or marks probably?) and see if there's any relevant testing. There might not be any good testing for this sort of thing currently.

__style: '',
__textFormat: 0,
__textStyle: '',
Expand All @@ -150,6 +151,7 @@ describe('table selection', () => {
__parent: 'root',
__prev: null,
__size: 1,
__state: {},
__style: '',
__textFormat: 0,
__textStyle: '',
Expand All @@ -163,6 +165,7 @@ describe('table selection', () => {
__next: null,
__parent: paragraphKey,
__prev: null,
__state: {},
__style: '',
__text: 'Hello world',
__type: 'text',
Expand Down
43 changes: 42 additions & 1 deletion packages/lexical-website/docs/concepts/node-replacement.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,12 +1,53 @@
# Node Customization
Copy link
Contributor Author

@GermanJablo GermanJablo Jan 31, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

I don't know how you guys handle redirects on the website, but I think it would be nice to rename the url to this title.


Originally the only way to customize nodes was using the node replacement API. Recently we have introduced a second way with the `state` property which has some advantages described below.

## State (New)

The advantages of using state over the replacement API are:
1. Easier (less boilerplate)
2. Composable (multiple plugins extending the same node causes failures)
3. Allows metadata: useful for adding things to the RootNode.

All you need to do is define keys with `createStateKey`, and then use it with the `setState` and `getState` methods.

```ts
// IMPLEMENTATION
const color = createStateKey('color', { parse: (value) => value as string });
GermanJablo marked this conversation as resolved.
Show resolved Hide resolved

// USAGE
const textNode = new TextNode();
GermanJablo marked this conversation as resolved.
Show resolved Hide resolved
textNode.setState(color, "blue");
const textColor = textNode.getState(color) // -> "blue"
```

Inside state, you can put any serializable json value except null. Our recommendation is to always use TypeScript in strict mode, so you don't have to worry about these things!

### Important

we recommend that you use prefixes with low collision probability when defining state keys. For example, if you are making a plugin called `awesome-lexical`, you could do:

```ts
const color = createStateKey('awesome-lexical-color', /** your parse fn */)
const bgColor = createStateKey('awesome-lexical-bg-color', /** your parse fn */)

// Or you can add all your state inside an object:
type AwesomeLexical = {
color?: string;
bgColor?: string;
padding?: number
}
const awesomeLexical = createStateKey('awesome-lexical', /** your parse fn which returns AwesomeLexical type */)

```

# Node Overrides / Node Replacements

Some of the most commonly used Lexical Nodes are owned and maintained by the core library. For example, ParagraphNode, HeadingNode, QuoteNode, List(Item)Node etc - these are all provided by Lexical packages, which provides an easier out-of-the-box experience for some editor features, but makes it difficult to override their behavior. For instance, if you wanted to change the behavior of ListNode, you would typically extend the class and override the methods. However, how would you tell Lexical to use *your* ListNode subclass in the ListPlugin instead of using the core ListNode? That's where Node Overrides can help.

Node Overrides allow you to replace all instances of a given node in your editor with instances of a different node class. This can be done through the nodes array in the Editor config:

```
```ts
const editorConfig = {
...
nodes=[
Expand Down
60 changes: 60 additions & 0 deletions packages/lexical/src/LexicalNode.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -59,6 +59,7 @@ export type SerializedLexicalNode = {
type: string;
/** A numeric version for this schema, defaulting to 1, but not generally recommended for use */
version: number;
state?: State;
};

/**
Expand Down Expand Up @@ -184,6 +185,39 @@ export type DOMExportOutput = {

export type NodeKey = string;

type DeepImmutable<T> = T extends Map<infer K, infer V>
GermanJablo marked this conversation as resolved.
Show resolved Hide resolved
? ReadonlyMap<DeepImmutable<K>, DeepImmutable<V>>
: T extends Set<infer S>
? ReadonlySet<DeepImmutable<S>>
: T extends object
? {
readonly [K in keyof T]: DeepImmutable<T[K]>;
}
: T;
type State = {[Key in string]?: string | number | boolean | State};
type StateValue = string | number | boolean | State;
interface StateKey<
K extends string = string,
V extends StateValue = StateValue,
> {
key: K;
// Here we are storing a default for convenience
value: DeepImmutable<V>;
parse: (value: unknown) => V;
GermanJablo marked this conversation as resolved.
Show resolved Hide resolved
}
interface StateKeyConfig<V extends StateValue = StateValue> {
parse: (value: unknown) => V;
GermanJablo marked this conversation as resolved.
Show resolved Hide resolved
}
type StateKeyConfigValue<T extends StateKeyConfig> = ReturnType<T['parse']>;

// eslint-disable-next-line @typescript-eslint/no-explicit-any
export function createStateKey<K extends string, T extends StateKeyConfig<any>>(
key: K,
config: T,
): StateKey<K, StateKeyConfigValue<T>> {
return {key, parse: config.parse, value: config.parse(undefined)};
}

export class LexicalNode {
// Allow us to look up the type including static props
['constructor']!: KlassConstructor<typeof LexicalNode>;
Expand All @@ -198,6 +232,19 @@ export class LexicalNode {
__prev: null | NodeKey;
/** @internal */
__next: null | NodeKey;
/** @internal */
__state: DeepImmutable<State> = {};
GermanJablo marked this conversation as resolved.
Show resolved Hide resolved

getState<T extends StateKey>(k: T): T['value'] | undefined {
const self = this.getLatest();
// If the state is not set, return the default value
return self.__state[k.key] ?? k.parse(undefined);
Copy link
Collaborator

Choose a reason for hiding this comment

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

With a single object implementation this must always call parse because we don’t know if its directly from JSON or not

GermanJablo marked this conversation as resolved.
Show resolved Hide resolved
}

setState<T extends StateKey>(k: T, v: T['value']) {
const self = this.getWritable();
(self.__state as State)[k.key] = v;
GermanJablo marked this conversation as resolved.
Show resolved Hide resolved
}

// Flow doesn't support abstract classes unfortunately, so we can't _force_
// subclasses of Node to implement statics. All subclasses of Node should have
Expand Down Expand Up @@ -885,6 +932,7 @@ export class LexicalNode {
return {
type: this.__type,
version: 1,
...(objetcIsEmpty(this.__state) ? {} : {state: this.__state}),
};
}

Expand Down Expand Up @@ -934,6 +982,7 @@ export class LexicalNode {
updateFromJSON(
serializedNode: LexicalUpdateJSON<SerializedLexicalNode>,
): this {
this.__state = serializedNode.state || {};
Copy link
Collaborator

Choose a reason for hiding this comment

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

This logic also needs to go into afterCloneFrom, and probably worth adding tests to make sure that this feature doesn’t rewrite history (setting state shouldn’t affect previous reconciled versions of the node)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Any idea how to test it? I tried comparing _editorState with _pendingEditorState without success

Copy link
Collaborator

Choose a reason for hiding this comment

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

let v0: RootNode;
let v1: RootNode;
const vk = createStateKey('vk', { parse: (v) => typeof v === 'number' ? v : null });
editor.update(
  () => {
    v0 = $getRoot();
    v0.setState(vk, 0);
    expect(v0.getState(vk)).toBe(0);
  },
  {discrete: true},
);
const state0 = editor.getEditorState();
editor.update(
  () => {
    v0.setState(vk, 1);
    // normally would use chaining for this but setState doesn't return this
    v1 = v0.getLatest();
    // This is testing getLatest()
    expect(v0.getState(vk)).toBe(1);
    expect(v1.getState(vk)).toBe(1);
    expect(v1.is(v0)).toBe(true);
  },
  {discrete: true},
);
const state1 = editor.getEditorState();
// Test that the correct version is returned and that they are independent
expect(state0.read(() => v0.getState(vk))).toBe(0);
expect(state1.read(() => v1.getState(vk))).toBe(1);
// Test that getLatest is used and not the __state property directly
expect(state0.read(() => v1.getState(vk))).toBe(0);
expect(state1.read(() => v0.getState(vk))).toBe(1);

return this;
}

Expand Down Expand Up @@ -1289,3 +1338,14 @@ export function insertRangeAfter(
currentNode = currentNode.insertAfter(nodeToInsert);
}
}

/**
* The best way to check if an object is empty in O(1)
* @see https://stackoverflow.com/a/59787784/10476393
*/
function objetcIsEmpty(obj: object) {
GermanJablo marked this conversation as resolved.
Show resolved Hide resolved
for (const key in obj) {
return false;
}
return true;
}
9 changes: 9 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 @@ -1206,6 +1206,7 @@ describe('LexicalEditor tests', () => {
__parent: null,
__prev: null,
__size: 1,
__state: {},
__style: '',
__textFormat: 0,
__textStyle: '',
Expand All @@ -1222,6 +1223,7 @@ describe('LexicalEditor tests', () => {
__parent: 'root',
__prev: null,
__size: 0,
__state: {},
__style: '',
__textFormat: 0,
__textStyle: '',
Expand Down Expand Up @@ -1295,6 +1297,7 @@ describe('LexicalEditor tests', () => {
__parent: null,
__prev: null,
__size: 1,
__state: {},
__style: '',
__textFormat: 0,
__textStyle: '',
Expand All @@ -1311,6 +1314,7 @@ describe('LexicalEditor tests', () => {
__parent: 'root',
__prev: null,
__size: 1,
__state: {},
__style: '',
__textFormat: 0,
__textStyle: '',
Expand All @@ -1324,6 +1328,7 @@ describe('LexicalEditor tests', () => {
__next: null,
__parent: paragraphKey,
__prev: null,
__state: {},
__style: '',
__text: 'Hello world',
__type: 'text',
Expand Down Expand Up @@ -1379,6 +1384,7 @@ describe('LexicalEditor tests', () => {
__parent: null,
__prev: null,
__size: 1,
__state: {},
__style: '',
__textFormat: 0,
__textStyle: '',
Expand All @@ -1395,6 +1401,7 @@ describe('LexicalEditor tests', () => {
__parent: 'root',
__prev: null,
__size: 1,
__state: {},
__style: '',
__textFormat: 0,
__textStyle: '',
Expand All @@ -1408,6 +1415,7 @@ describe('LexicalEditor tests', () => {
__next: null,
__parent: paragraphKey,
__prev: null,
__state: {},
__style: '',
__text: 'Hello world',
__type: 'text',
Expand Down Expand Up @@ -2900,6 +2908,7 @@ describe('LexicalEditor tests', () => {
__next: null,
__parent: null,
__prev: null,
__state: {},
__style: '',
__text: 'yolo',
__type: 'text',
Expand Down
4 changes: 4 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 @@ -62,6 +62,7 @@ describe('LexicalEditorState tests', () => {
__parent: null,
__prev: null,
__size: 1,
__state: {},
__style: '',
__textFormat: 0,
__textStyle: '',
Expand All @@ -78,6 +79,7 @@ describe('LexicalEditorState tests', () => {
__parent: 'root',
__prev: null,
__size: 1,
__state: {},
__style: '',
__textFormat: 0,
__textStyle: '',
Expand All @@ -91,6 +93,7 @@ describe('LexicalEditorState tests', () => {
__next: null,
__parent: '1',
__prev: null,
__state: {},
__style: '',
__text: 'foo',
__type: 'text',
Expand Down Expand Up @@ -150,6 +153,7 @@ describe('LexicalEditorState tests', () => {
__parent: null,
__prev: null,
__size: 0,
__state: {},
__style: '',
__textFormat: 0,
__textStyle: '',
Expand Down
Loading