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.

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

Add node replacement system to core #3367

Merged
merged 5 commits into from
Nov 18, 2022
Merged

Add node replacement system to core #3367

merged 5 commits into from
Nov 18, 2022

Conversation

trueadm
Copy link
Collaborator

@trueadm trueadm commented Nov 16, 2022

This PR extends on the work from #2924.

Instead of defining a proxy that the user must remember to invoke, we can instead opt nodes into replacement in their factory function $createXNode using the new exported function $applyNodeReplacement. Then when defining the nodes array for editor config, we can supply an object in the form of:

{
  nodes: […SomeNodes, { replace: MyNode, with(node) { return $createMyNode() }]
}

This should address the issues on the above PR.

@vercel
Copy link

vercel bot commented Nov 16, 2022

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

Name Status Preview Updated
lexical ✅ Ready (Inspect) Visit Preview Nov 17, 2022 at 4:15PM (UTC)
lexical-playground ✅ Ready (Inspect) Visit Preview Nov 17, 2022 at 4:15PM (UTC)

@facebook-github-bot 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 Nov 16, 2022
@fantactuka
Copy link
Contributor

fantactuka commented Nov 17, 2022

Should name be more generic to allow easier extension in future? Eg if we introduce something else than replace/with options $nodeFactory/$createNode/etc

@trueadm
Copy link
Collaborator Author

trueadm commented Nov 17, 2022

I'm not sure we'd ever want to introduce $nodeFactory/$createNode as it would be such a large breaking change that it would cause a ton of churn for no real gains.

Copy link
Member

@zurfyx zurfyx left a comment

Choose a reason for hiding this comment

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

Thank you @trueadm! Stamping to unblock but I share the same sentiment as @fantactuka. I feel like a more generic name (just the term) would give us space to expand this in the future with further validation or other stuff without a breaking changes. I am personally for $createNote

@@ -1263,3 +1263,29 @@ export function $copyNode<T extends LexicalNode>(node: T): T {
$setNodeKey(copy, null);
return copy;
}

export function $applyNodeReplacement<N extends LexicalNode>(
node: LexicalNode,
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
node: LexicalNode,
node: N,

This will guarantee that the return is the same or an extended node?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

They aren't the same though. The extended node is a subclass of N, not N.

@trueadm
Copy link
Collaborator Author

trueadm commented Nov 18, 2022

@zurfyx I don't understand how $createNote would work here, can you explain? Are you suggesting removing the new SomeNode pattern? If so, that's not going to scale or work with type systems.

@zurfyx
Copy link
Member

zurfyx commented Nov 18, 2022

@zurfyx I don't understand how $createNote would work here, can you explain? Are you suggesting removing the new SomeNode pattern? If so, that's not going to scale or work with type systems.

I was thinking that something like $createNode(() => new SomeNode) could maybe give us more flexibility later. But in any case, what you propose is still extensible later if we need to

@trueadm
Copy link
Collaborator Author

trueadm commented Nov 18, 2022

$createNode(() => new SomeNode) doesn't seem to do what it says, as in you can still create the node without using $createNode, which means the lines are blurred.

@acywatson
Copy link
Contributor

This looks right. Nice work.

@acywatson
Copy link
Contributor

Actually what happens if I override TextNode or something and then copy and paste into a Lexical editor that doesn't have that override? I guess we'll fallback to HTML. Maybe that's the best we can do here.

@trueadm trueadm merged commit 102b01d into main Nov 18, 2022
@trueadm trueadm deleted the node-replacement branch November 18, 2022 17:11
thegreatercurve pushed a commit that referenced this pull request Nov 25, 2022
* Add node replacement system to core

* Add test

* Fix missing logic

* Fix prettier

* Update flow types
NhanHo added a commit to NhanHo/lexical that referenced this pull request Dec 18, 2022
@thegreatercurve thegreatercurve mentioned this pull request Dec 20, 2022
@nerdess
Copy link

nerdess commented May 4, 2023

@trueadm can you please post a concrete example on how to use this:

{
  nodes: […SomeNodes, { replace: MyNode, with(node) { return $createMyNode() }]
}

E.g. I want to replace the ParagraphNode with a custom version but I struggle how to inject my ParagraphNodeCustom into Lexical.


edit: actually, I just found this in the docs and the code in the sandbox is exactly what I was looking for: https://lexical.dev/docs/concepts/node-replacement

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.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants