Skip to content

Add node replacement system to core - #3367

Merged
trueadm merged 5 commits into
mainfrom
node-replacement
Nov 18, 2022
Merged

trueadm merged 5 commits into
mainfrom
node-replacement

Conversation

@trueadm

@trueadm trueadm commented Nov 16, 2022 •

Copy link
Copy Markdown
Collaborator

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

vercel Bot commented Nov 16, 2022 •

Copy link
Copy Markdown

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

fantactuka commented Nov 17, 2022 •

Copy link
Copy Markdown
Contributor

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

trueadm commented Nov 17, 2022

Copy link
Copy Markdown
Collaborator Author

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.

@zurfyx zurfyx left a comment •

Copy link
Copy Markdown
Member

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

}

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

Copy link
Copy Markdown
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
Copy Markdown
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

trueadm commented Nov 18, 2022

Copy link
Copy Markdown
Collaborator Author

@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

zurfyx commented Nov 18, 2022

Copy link
Copy Markdown
Member

@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

trueadm commented Nov 18, 2022

Copy link
Copy Markdown
Collaborator Author

$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
Copy Markdown
Contributor

This looks right. Nice work.

@acywatson

Copy link
Copy Markdown
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

nerdess commented May 4, 2023 •

Copy link
Copy Markdown

@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

This branch was successfully deployed

2 active deployments
Preview – lexical-playground — f02f86f9 Deployed Nov 17, 2022 by vercel[bot]
Preview – lexical — f02f86f9 Deployed Nov 17, 2022 by vercel[bot]
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.

6 participants