WIP: Generic ImportJSON/ExportJSON #3931

GermanJablo · 2023-02-17T18:36:57Z

This is the second in a series of changes I proposed in #3763 to simplify Lexical, remove code duplication, and improve DX.

In short, thanks to this PR, no node needs to define an importJSON or exportJSON method.

The most important thing (and the first thing I recommend checking) are the changes in LexicalUpdate.ts (for importJSON) and in lexicalNode.ts (for exportJSON).

Pending:

The mode, direction and format properties mismatch between the way they are exported and imported making the algorithm more inefficient. I have a PR in mind that would improve this. Either way, the PR as it is is now functional.
There are currently 5 unit tests failing because exportJSON seems to work fine, it just changes the order of the keys. My question is, is it okay if I modify those tests to compare objects instead of the stringified editorState?

vercel · 2023-02-17T18:37:02Z

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name	Status	Preview	Comments	Updated
lexical	✅ Ready (Inspect)	Visit Preview	💬 Add your feedback	Feb 26, 2023 at 6:22PM (UTC)
lexical-playground	✅ Ready (Inspect)	Visit Preview	💬 Add your feedback	Feb 26, 2023 at 6:22PM (UTC)

GermanJablo · 2023-02-17T18:57:01Z

packages/lexical/src/LexicalUpdates.ts

+  const serializedNode2 = JSON.parse(JSON.stringify(serializedNode));
+  delete serializedNode2.children;
+  delete serializedNode2.version;
+  delete serializedNode2.mode;
+  delete serializedNode2.direction;
+  delete serializedNode2.format;
+  let node = new registeredNode.klass();
+  // @ts-expect-error
+  node.setFormat(serializedNode.format);
+  if ($isTextNode(node)) {
+    // @ts-expect-error
+    node.setMode(serializedNode.mode);
  }
-
-  const node = nodeClass.importJSON(serializedNode);
+  if (
+    $isElementNode(node) &&
+    type !== 'tablecell' &&
+    type !== 'tablerow' &&
+    type !== 'table'
+  ) {
+    // @ts-expect-error
+    node.setDirection(serializedNode.direction);
+  }
+  const prefix = '__';
+  const withUnderscore = Object.fromEntries(
+    Object.entries(serializedNode2).map(([k, v]) => [`${prefix}${k}`, v]),
+  );
+  node = Object.assign(node, withUnderscore);


This is the most important part which replaces importJSON.

As a side note, one of the ideas I have for simplifying setters and getters is to use a generic proxy that accesses getLatest() and getWritable(), so probably the underscore __ thing may not be necessary anymore. here or anywhere.

acywatson · 2023-02-17T19:25:02Z

I’m not sure this makes sense. The purpose of this API is to allow custom nodes to control deserialization behavior, which allows for runtime backwards compatibility for Lexical states in storage. With this change, you’ve taken that flexibility and control away from developers and instead created a place in the core that will have to be continuously expanded and special-cased as new core node types are added.

GermanJablo · 2023-02-17T22:14:07Z

packages/lexical/src/LexicalNode.ts

+  exportJSON() {
+    let serializedNode = JSON.parse(JSON.stringify(this.getLatest()));
+    delete serializedNode.__first;
+    delete serializedNode.__last;
+    delete serializedNode.__size;
+    delete serializedNode.__parent;
+    delete serializedNode.__next;
+    delete serializedNode.__prev;
+    delete serializedNode.__cachedText;
+    delete serializedNode.__key;
+    delete serializedNode.__dir;
+    serializedNode = Object.fromEntries(
+      Object.entries(serializedNode).map(([k, v]) => [k.slice(2), v]),
    );
+    if ($isElementNode(this)) {
+      serializedNode = {children: [], ...serializedNode};
+      serializedNode.direction = this.getDirection();
+      serializedNode.format = this.getFormatType();
+      serializedNode.indent = this.getIndent();
+    }
+    if ($isTextNode(this)) {
+      serializedNode.mode = this.getMode();
+    }
+    serializedNode.type = this.getType();
+    serializedNode.version = 1;
+    return serializedNode;


This is the other important part, which replaces exportJSON()

GermanJablo · 2023-02-17T22:20:40Z

I've updated the PR to add exportJSON. As you can see, the new exportJSON method of LexicalNode is generic, but a node could override it by inheritance if someone wanted to.

I can do the same with importJSON.

What do you think @acywatson?

acywatson · 2023-02-17T22:28:13Z

I've updated the PR to add exportJSON. As you can see, the new exportJSON method of LexicalNode is generic, but a node could override it by inheritance if someone wanted to.

I can do the same with importJSON.

What do you think @acywatson?

My recommendation would be for you to review this PR: #2157

The reason serialization works like this is not because we never thought about just serializing all the properties on the node - that was basically how it was done before. The idea was to create a type-safe abstraction between the serialized types and the internal node structures to insulate against changes to those internal structure breaking old stored Lexical states.

I appreciate the effort, but I don't think we have a lot of appetite to revisit this right now.

GermanJablo · 2023-02-18T08:41:34Z

I have found in the last commit a way to autogenerate the types of the serialized nodes automatically. As you can see in the video, it works with inheritance.

https://www.loom.com/share/4601d7d7411c4eafbfd47d2281bade52

Maybe that solves the problem?

I appreciate the effort, but I don't think we have a lot of appetite to revisit this right now.

I believe that what I propose is viable and has enormous advantages. But if you don't find value in these changes, please feel free to close the PR. I'd rather use a fork than be a nuisance :)

GermanJablo · 2023-02-18T10:37:49Z

packages/lexical/src/LexicalNode.ts

-  // eslint-disable-next-line @typescript-eslint/no-explicit-any
-  [x: string]: any;


This conflicts with the solution I found for serialized node types.
I don't understand why it was done in the first instance. It doesn't seem like a good practice since it gives a false illusion of type-safety, when in fact it breaks it at the beginning of the class hierarchy.

acywatson · 2023-02-18T15:04:37Z

I believe that what I propose is viable and has enormous advantages. But if you don't find value in these changes, please feel free to close the PR. I'd rather use a fork than be a nuisance :)

@egonbolton You're not being a nuisance, but I'm also perfectly happy for you to use a fork - it is open source, after all :)

Let me spend some time with your work and see if I can get comfortable with the functionality in the scenarios I care about. Specifically, I need to understand what happens when someone decides to change a node to add a new property in. How does deserialization work with old states that don't have that property? What about when a new state (containing the added property) is deserialized by an old editor? How does the type system behave when the user makes changes to the public serialization interface of a node so that they're warned about the two scenarios above and can react accordingly?

I'm not saying that your proposal doesn't have advantages (they might even be enormous ones!), but it is potentially very consequential for all of our internal systems, which means I need to spend my time verifying it, which impacts my other priorities. That's the only issue here - I'm happy to consider the idea on it's merits and we're better just for having seen the idea, whether we merge it now or not.

I have some free time this weekend, so I will try to check out the branch and verify some of these scenarios and see where I get.

packages/lexical/src/__tests__/unit/LexicalSerialization.test.ts

packages/lexical/src/__tests__/unit/LexicalEditorState.test.ts

packages/lexical-website/docs/concepts/serialization.md

packages/lexical-link/src/index.ts

fantactuka · 2023-02-25T18:21:58Z

packages/lexical/src/LexicalUpdates.ts

+      delete serializedNode2.format;
+      node.setFormat(serializedNode.format);
+      if (type !== 'tablecell' && type !== 'tablerow' && type !== 'table') {
+        node.setDirection(serializedNode.direction);


Would it be better to override these nodes' setDirection to be noop?

You mean to export the direction in those nodes right? I agree.
If that's okay with you, I'll have to modify the unit tests to include that property just like I had to do with colSpan

packages/lexical-website/docs/concepts/serialization.md

fantactuka · 2023-02-25T22:12:04Z

packages/lexical/src/LexicalNode.ts

-      'LexicalNode: Node %s does not implement .importJSON().',
-      this.name,
+  exportJSON() {
+    let serializedNode = JSON.parse(JSON.stringify(this.getLatest()));


Tried to run it on a doc with 5k nodes, got ~35ms vs ~6ms on main branch, it's not extremely slow, but I can imagine someone to have editor.toJSON within onChange handler. Tried to replace it with looping through prefixed keys and it works better (~9ms) but it leaves more responsibility for custom nodes owners, who need to ensure their props are serializable or provide own import/export overrides. We can try dev mode invariants that will warn about non-serializable values, but won't affect production users

Excellent improvement, thank you very much!

Tried to replace it with looping through prefixed keys and it works better (~9ms) but it leaves more responsibility for custom nodes owners, who need to ensure their props are serializable or provide own import/export overrides.

If I'm not misunderstanding, that's true for both my implementation and the variation you've made. The purpose of this PR in principle is that it is only necessary to define serialization on non-serializable nodes.

Actually, I believe that automatic serialization could always be achieved by using something like devalue. However, I'm not sure what the implications of this are on performance or memory. Taking caption for example, I don't know if it would be good to serialize all editor properties like pendingEditorState and such. Maybe we can open a separate thread to discuss this separately?

Actually, I believe that automatic serialization could always be achieved by using something like devalue. However, I'm not sure what the implications of this are on performance or memory.

Automatic serialization is not necessarily what we want - more like flexible serialization with sane defaults.

Lexical core has no dependencies on any other library - we don't want to change that. Control over performance is one of the main reasons that was done.

Taking caption for example, I don't know if it would be good to serialize all editor properties like pendingEditorState and such.

I don't think it would be good. In the best case it's unnecessary bloat in editor states, in the worst case it encourages depending on implementation details that might change.

Yeah, that's what I thought @acywatson

@fantactuka, did you get it to work with your snippet?

even setting direction, indent and version there are some tests that break when I implement it.

@fantactuka, did you get it to work with your snippet?

No, I didn't get to the point to run testes over it, can take a look closer tomorrow evening

Lexical core has no dependencies on any other library - we don't want to change that.

Also adds 2.9kb to core

We could limit auto-serialization to primitives (numbers, strings, bools, nulls) and nested editors (by just calling .toJSON on it). Everything else should require custom import/export. It should be possible to lint or dev-mode-warn (class with no import/exportJSON and non-primitive props starting with "__"). I might be wrong, but I can only recall MarkNode and its sub-classes that use arrays/maps as serializable props

GermanJablo added 2 commits February 17, 2023 14:24

core change

ffd4532

remove unnecessary code

ac2aa99

GermanJablo requested review from zurfyx, fantactuka, acywatson, tylerjbainbridge and thegreatercurve as code owners February 17, 2023 18:36

facebook-github-bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Feb 17, 2023

vercel bot deployed to Preview – lexical February 17, 2023 18:38 View deployment

vercel bot deployed to Preview – lexical-playground February 17, 2023 18:40 View deployment

GermanJablo commented Feb 17, 2023

View reviewed changes

GermanJablo added 3 commits February 17, 2023 17:50

core change for exportJSON

23725d9

refactor core changes of exportJSON()

c975cc6

remove unnecessary code

a242121

GermanJablo changed the title ~~Generic ImportJSON~~ Generic ImportJSON/ExportJSON Feb 17, 2023

vercel bot deployed to Preview – lexical February 17, 2023 22:05 View deployment

vercel bot deployed to Preview – lexical-playground February 17, 2023 22:06 View deployment

GermanJablo commented Feb 17, 2023

View reviewed changes

GermanJablo mentioned this pull request Feb 17, 2023

Generalize setters/getters, clone, importJSON and exportJSON #3763

Open

autogenerated serialized node types

247e823

vercel bot had a problem deploying to Preview – lexical February 18, 2023 08:27 Failure

vercel bot had a problem deploying to Preview – lexical-playground February 18, 2023 08:27 Failure

GermanJablo commented Feb 18, 2023

View reviewed changes

save changes

d5aba2a

versioning docs

205f640

vercel bot had a problem deploying to Preview – lexical-playground February 25, 2023 13:10 Failure

vercel bot had a problem deploying to Preview – lexical February 25, 2023 13:10 Failure

GermanJablo commented Feb 25, 2023

View reviewed changes

packages/lexical/src/__tests__/unit/LexicalSerialization.test.ts Show resolved Hide resolved

GermanJablo commented Feb 25, 2023

View reviewed changes

packages/lexical/src/__tests__/unit/LexicalEditorState.test.ts Show resolved Hide resolved

GermanJablo commented Feb 25, 2023

View reviewed changes

packages/lexical-website/docs/concepts/serialization.md Outdated Show resolved Hide resolved

GermanJablo commented Feb 25, 2023

View reviewed changes

packages/lexical-link/src/index.ts Show resolved Hide resolved

fix types

aba5306

vercel bot had a problem deploying to Preview – lexical-playground February 25, 2023 13:53 Failure

vercel bot had a problem deploying to Preview – lexical February 25, 2023 13:54 Failure

integrity error

52a2462

vercel bot had a problem deploying to Preview – lexical-playground February 25, 2023 14:03 Failure

vercel bot had a problem deploying to Preview – lexical February 25, 2023 14:03 Failure

fix type error?

7788bf2

vercel bot deployed to Preview – lexical February 25, 2023 14:18 View deployment

vercel bot deployed to Preview – lexical-playground February 25, 2023 14:19 View deployment

fantactuka reviewed Feb 25, 2023

View reviewed changes

improve docs

68336bd

vercel bot deployed to Preview – lexical February 26, 2023 04:07 View deployment

vercel bot deployed to Preview – lexical-playground February 26, 2023 04:08 View deployment

fix e2e error with mentionNode

afc48b5

vercel bot deployed to Preview – lexical February 26, 2023 15:10 View deployment

vercel bot deployed to Preview – lexical-playground February 26, 2023 15:11 View deployment

restore deleted test

cec3086

vercel bot deployed to Preview – lexical February 26, 2023 18:21 View deployment

vercel bot deployed to Preview – lexical-playground February 26, 2023 18:22 View deployment

GermanJablo mentioned this pull request Apr 9, 2023

Generic clone method #3920

Open

This was referenced Nov 23, 2023

Bug: lexical-yjs create nodes using constructor without args #4912

Open

Improve TypeScript types by removing [k: string]: any from LexicalNode #5223

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

WIP: Generic ImportJSON/ExportJSON #3931

WIP: Generic ImportJSON/ExportJSON #3931

GermanJablo commented Feb 17, 2023 •

edited

vercel bot commented Feb 17, 2023 •

edited

GermanJablo Feb 17, 2023 •

edited

acywatson commented Feb 17, 2023

GermanJablo Feb 17, 2023

GermanJablo commented Feb 17, 2023 •

edited

acywatson commented Feb 17, 2023

GermanJablo commented Feb 18, 2023

GermanJablo Feb 18, 2023

acywatson commented Feb 18, 2023

fantactuka Feb 25, 2023

GermanJablo Feb 26, 2023

fantactuka Feb 25, 2023 •

edited

GermanJablo Feb 26, 2023

acywatson Feb 26, 2023

GermanJablo Feb 26, 2023

fantactuka Feb 27, 2023 •

edited

		// eslint-disable-next-line @typescript-eslint/no-explicit-any
		[x: string]: any;

WIP: Generic ImportJSON/ExportJSON #3931

Are you sure you want to change the base?

WIP: Generic ImportJSON/ExportJSON #3931

Conversation

GermanJablo commented Feb 17, 2023 • edited

Pending:

vercel bot commented Feb 17, 2023 • edited

GermanJablo Feb 17, 2023 • edited

Choose a reason for hiding this comment

acywatson commented Feb 17, 2023

GermanJablo Feb 17, 2023

Choose a reason for hiding this comment

GermanJablo commented Feb 17, 2023 • edited

acywatson commented Feb 17, 2023

GermanJablo commented Feb 18, 2023

GermanJablo Feb 18, 2023

Choose a reason for hiding this comment

acywatson commented Feb 18, 2023

fantactuka Feb 25, 2023

Choose a reason for hiding this comment

GermanJablo Feb 26, 2023

Choose a reason for hiding this comment

fantactuka Feb 25, 2023 • edited

Choose a reason for hiding this comment

GermanJablo Feb 26, 2023

Choose a reason for hiding this comment

acywatson Feb 26, 2023

Choose a reason for hiding this comment

GermanJablo Feb 26, 2023

Choose a reason for hiding this comment

fantactuka Feb 27, 2023 • edited

Choose a reason for hiding this comment

GermanJablo commented Feb 17, 2023 •

edited

vercel bot commented Feb 17, 2023 •

edited

GermanJablo Feb 17, 2023 •

edited

GermanJablo commented Feb 17, 2023 •

edited

fantactuka Feb 25, 2023 •

edited

fantactuka Feb 27, 2023 •

edited