[lexical] Refactor: [RFC] LexicalNode.afterCloneFrom to simplify clone implementation

[etrepum]

Description

The code path for cloning lexical nodes is a bit weird with some branching for different node types and no extensibility. We can clean it up by moving that part of the clone process to an instance method.

Unlike #3920 this does not try and make a generic clone method, the boilerplate is still there – but all for good reason. It's at least 2-3x faster to have the boilerplate than to use Object.create, Object.assign, etc. presumably due to JIT de-optimization reasons. Using the constructor in this way (the status quo) also prevents sharing of mutable objects that could happen unintentionally with a shallow clone of all properties.

This changes the $cloneWithProperties function to first call latestNode.constructor.clone(latestNode), then call mutableNode.afterCloneFrom(latestNode) to handle all of the post-construction state updates. This means the clone static method can "simply" call the constructor and any other update to the node can be done in afterCloneFrom in the correct order (super first) rather than have it happen later in a way where users have no visibility or control over the process.

The change is entirely backwards compatible, other than reserving a new name on the API.

Test plan

Will write additional docs and tests if this is deemed to be a worthwhile endeavor.

[vercel]

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

Name	Status	Preview	Comments	Updated (UTC)
lexical	✅ Ready (Inspect)	Visit Preview	💬 Add feedback	Aug 12, 2024 2:55pm
lexical-playground	✅ Ready (Inspect)	Visit Preview	💬 Add feedback	Aug 12, 2024 2:55pm

[github-actions]

size-limit report 📦

Path	Size
lexical - cjs	29.39 KB (0%)
lexical - esm	29.17 KB (0%)
@lexical/rich-text - cjs	37.74 KB (0%)
@lexical/rich-text - esm	31.02 KB (0%)
@lexical/plain-text - cjs	36.32 KB (0%)
@lexical/plain-text - esm	28.37 KB (0%)
@lexical/react - cjs	39.65 KB (0%)
@lexical/react - esm	32.47 KB (0%)

[zurfyx]

This is better than the hardcoded version, but now we're saying that everyone has to implement a clone and afterCloneFrom? Because theoretically every Node can be inherited.

Have we considered automating the static clone and declaring the properties to clone via this other method so that users still only have to implement one? Similarly, we'd have to advocate for no constructor props (except for NodeKey), but if this happens it also indirectly fixes the reflection issue we're having on collab.

[etrepum]

afterCloneFrom is not usually needed, because most of the time there's not a lot of state that isn't already set in the constructor. You can see that it was used very sparingly but all the tests still pass.

Other solutions that boil down to enumerating property names suffer from the problem where you can end up easily sharing state across node versions that you don't want to (arrays, maps, etc.), or you pay the performance penalty of doing a deep clone, or the complexity penalty of having one way to do it most of the time and another way if there is any non-primitive state.

For collab I'm not sure what's best because I haven't read through all of it, but maybe it should be using json to round-trip nodes. Maybe another method or methods could be added for performance reasons if necessary. Note that collab does have the versioning problem that the json serialization codec at least has the convention to solve, although probably ideally toJSON would have a version argument so new clients could force older serializations for backwards compatibility during an upgrade phase.

[zurfyx]

Other solutions that boil down to enumerating property names suffer from the problem where you can end up easily sharing state across node versions that you don't want to (arrays, maps, etc.), or you pay the performance penalty of doing a deep clone

@etrepum I do agree, which is why I was thinking on doubling down on your proposal to potentially drop the use of the constructor to pass parameters and having everything consolidated in one place. I haven't thought deeply about this and as usual, doesn't play great with backward compatibility.

I made a similar proposal when we introduced what is now $applyNodeReplacement. Instead of users being able to freely instantiate Nodes, we can instead make it more framework-like and have full control over the creation which would enable us to do things like some validation ahead of time.

[etrepum]

I don't think that really solves anything from a collab perspective, although making construction and init two phase (like Obj-C's +alloc -init) might reduce some boilerplate at a large backwards compatibility expense (and possibly perf regression if everything ends up going through getWritable setters). AFAICT the design of lexical-yjs is just fundamentally flawed, it should've always been using an explicit codec like the existing JSON one (or equivalent). Trying to sync all of the enumerable properties and not run through the normal constructor is just a bad solution. It seems like a given that we'll want to make a big API break for collab but I don't think it's as necessary or beneficial to force that on everything else.

Possibly we could have some utility node subclasses that could use slower reflection or deep cloning to make it easier for users' custom nodes to implement all of these protocols without breaking API compatibility, but leave the option open for people to work at the lower level that we have now that trades boilerplate for performance.

[zurfyx]

Nit, why is this called afterCloneFrom instead of afterClone?

[etrepum]

I called it afterCloneFrom to make it more clear what the argument was for and which instance to use

[etrepum]

Had to manually fix some conflicts in LexicalNode.test.ts so the review was dismissed