Skip to content

[lexical-list] Fix: create copies ListNode/ListItemNode in split-like operations#8213

Merged
etrepum merged 8 commits intofacebook:mainfrom
levensta:copynodes-in-list
Mar 16, 2026
Merged

[lexical-list] Fix: create copies ListNode/ListItemNode in split-like operations#8213
etrepum merged 8 commits intofacebook:mainfrom
levensta:copynodes-in-list

Conversation

@levensta
Copy link
Contributor

@levensta levensta commented Mar 12, 2026

Description

ListNode and ListItemNode internally create new list structures during operations such as splitting lists, indenting/outdenting. Previously, these operations relied on factory functions ($createListNode / $createListItemNode).

This caused issues when users extended ListNode or ListItemNode. Even if an editor contained custom subclasses, split-like operations would always create base ListNode / ListItemNode instances, breaking type consistency.

This change replaces factory-based creation with $copyNode, ensuring that new nodes are created from the same class as the source node.

Screen.Recording.2026-03-12.at.03.50.33.mov

https://codesandbox.io/p/sandbox/lexical-extended-list-rgrcq9

Test plan

Before

Split-like operations (list split, indent, outdent) always created base instances ListNode and ListItemNode. Even if the original list used subclasses.

After

Instances of the same class as the source node are now created using $copyNode in the following cases:

  • Splitting a list by pressing Enter inside an empty element
  • Handling indentation/outdentation
  • Using splice method
  • Using the replace method on a ListItem between other elements

This allows you to save custom ListNode and ListItemNode subclasses.

@vercel
Copy link

vercel bot commented Mar 12, 2026

The latest updates on your projects. Learn more about Vercel for GitHub.

Project Deployment Actions Updated (UTC)
lexical Ready Ready Preview, Comment Mar 15, 2026 9:44pm
lexical-playground Ready Ready Preview, Comment Mar 15, 2026 9:44pm

Request Review

@meta-cla meta-cla 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 Mar 12, 2026
@etrepum
Copy link
Collaborator

etrepum commented Mar 12, 2026

Using $copyNode is probably better than accessing the constructor directly

@etrepum
Copy link
Collaborator

etrepum commented Mar 12, 2026

Should also have unit tests that demonstrate that the change allows for the kind of functionality you’re trying to add

@etrepum etrepum added the extended-tests Run extended e2e tests on a PR label Mar 12, 2026
@levensta
Copy link
Contributor Author

Using $copyNode is probably better than accessing the constructor directly

Initially, I tried using $copyNode, but it makes a complete copy of all class properties, including node state. At the very least, this caused the saving of markdown list markers to break https://github.com/facebook/lexical/blob/main/packages/lexical-markdown/src/MarkdownTransformers.ts#L223-L225

@etrepum
Copy link
Collaborator

etrepum commented Mar 12, 2026

You'll have to be more specific about "saving of the list markers to break". Not quite sure what that means, it should just re-use the same style of list marker when this copy is made? If the markdown export doesn't check for mismatched list markers then that should be fixed.

There are some potential pitfalls with a full $copyNode (but if there are any you'd generally have the same problems when copy & paste is used), but a completely shallow copy has other potential pitfalls (missing state like format and such). Probably what should happen is that this functionality should be put in an instance method similar to afterCloneFrom or TextNode's splitText to isolate this copy-like functionality and have a place where the instance can be mutated accordingly after construction and if that behavior needs to be overridden in a subclass then super can be called to prevent code duplication.

@levensta
Copy link
Contributor Author

I am referring to a situation where a list is created using markdown, then a new item is created and indented. When indenting, a copy of ListItemNode is created, which also copies the listMarkerState, even though it is already part of another nested list

Screen.Recording.2026-03-12.at.12.02.05.mov
image

To create a copy using $copyNode, you need to reset all properties to their default state, but there is no way to reset the state using a method, except in the following way:

$copyNode(listItem).getWritable().__state = undefined;

That's why I thought it would be easier to create a copy using the constructor, since it doesn't require a verbose reset of all properties and states.

@etrepum
Copy link
Collaborator

etrepum commented Mar 12, 2026

That doesn’t seem broken to use the same marker for an indented list? Either way you’re either verbosely copying state from one node to the next or verbosely resetting it.

With either approach, using a method would allow this to be isolated and overridden if necessary. I don’t think adding direct constructor calls all over the place is a good idea, a few strategic places that are easy to find and maintain is ok.

@levensta
Copy link
Contributor Author

Either way you’re either verbosely copying state from one node to the next or verbosely resetting it.

I agree. If you need to avoid using a constructor, I'd suggest creating a helper function that hides the boilerplate code for resetting properties and state

function $spawnListItemNode(node) {
  const copy = $copyNode(node).setValue(1).setChecked(undefined);
  copy.getWritable().__state = undefined;
  return copy;
}

@etrepum
Copy link
Collaborator

etrepum commented Mar 12, 2026

I do not recommend resetting NodeState in this way.

@levensta
Copy link
Contributor Author

I do not recommend resetting NodeState in this way.

But are there other ways? $setState assumes an explicit prodvide of a known state

I looked at the internal available state API and maybe this approach is acceptable?

const state = $getWritableNodeState(node);
state.knownState.forEach((_, stateConfig) => {
   state.updateFromKnown(stateConfig, null);
});

I would also suggest using the $create function, but it also requires passing a constructor

@etrepum
Copy link
Collaborator

etrepum commented Mar 12, 2026

You’re misunderstanding me here, using the constructor is ok, but don’t spread it all over the code base. Put it in a method so it is easy to find for maintenance and can be overridden.

As for NodeState it shouldn’t be cleared at all, if specific properties should be omitted in this kind of copy-as-template then that’s functionality we’d want to add to the NodeState configuration and support it with some kind of copyNode-like functionality.

@levensta levensta force-pushed the copynodes-in-list branch from 0fc3f26 to 09213aa Compare March 13, 2026 07:44
@levensta levensta changed the title [lexical-list] Refactor: create copies ListNode/ListItemNode via the constructor in split-like operations [lexical][lexical-list] Feature: add create() factory method for ElementNode and use it for List split-like operations Mar 13, 2026
@etrepum
Copy link
Collaborator

etrepum commented Mar 16, 2026

Let's update the PR description to be a little more explicit about what this change is and how it affects various list operations before merge, this will help with the release notes

@levensta
Copy link
Contributor Author

Let's update the PR description to be a little more explicit about what this change is and how it affects various list operations before merge, this will help with the release notes

done

@etrepum etrepum added this pull request to the merge queue Mar 16, 2026
Merged via the queue into facebook:main with commit ed2fd08 Mar 16, 2026
46 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. extended-tests Run extended e2e tests on a PR

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants