Skip to content

Node Proxies - #2924

Closed
zurfyx wants to merge 1 commit into
mainfrom
factory
Closed

zurfyx wants to merge 1 commit into
mainfrom
factory

Conversation

@zurfyx

@zurfyx zurfyx commented Aug 31, 2022 •

Copy link
Copy Markdown
Member

👷‍♂️ This PR is a WIP

Screen.Recording.2022-08-31.at.2.38.21.pm.mov

Problem

You want to customize a previously defined node like TextNode or even within a plugin you don't control. Your options:

  1. Submit these customization to the original source (Lexical repo for TextNode)
  2. Patch it via transform and/or node mutation listeners

Neither of them are ideal. 1. may be rejected for very custom feature, 2. runs either after the update (in case of transform) or after the reconciliation cycle (in case of mutation listeners).

Ideally you want a mechanism that immediately and synchronously creates a YourCustomNode when OriginalNode is created. Such process should happen behind the scenes, work everywhere within the code and be transparent to the implementation.

Proposal

Proxies. A function, defined at editor creation time, capable of morphing nodes.

<MLCComposer initialConfig={
  nodes: [TextNode, ACTextNode],
  proxies: [
    [TextNode, (textNode: TextNode) => {
        const node = $createACTextNode(textNode.__text);
        node.setFormat(textNode.format);
        node.setDetail(textNode.detail);
        node.setMode(textNode.mode);
        node.setStyle(textNode.style);
        return node;
      }]
  ]}>

Every time a Textnode is created it will immediately be replaced by an ACTextNode.

Unfortunately, we can't make it automatic out of the box, $createTextNode will always create a TextNode and so will `new TextNode. However, for those pro users who require Proxies we can use a wrapper function that will facilitate this instead.

$withNodeProxy(() => $createTextNode());

Food for thought

Obviously, the $withNodeProxy is verbose and very user friendly. While it's true that it is specifically designed to be used by library created and advanced users we can possible do better.

$createNode(ParagraphNode);

Where $createNode will performs all this proxying behind the scenes.

This is major breaking change as it would imply getting rid of constructor properties and deprecating the use of new. However, it would also fix collab specific issues when we populate nodes by reflection and enforce a standardized way of node creation.


Thanks @trueadm for the factory and $ function proposals in #1262 and offline

Closes #1262

@vercel

vercel Bot commented Aug 31, 2022

Copy link
Copy Markdown

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

Name Status Preview Updated
lexical ❌ Failed (Inspect) Aug 31, 2022 at 7:03PM (UTC)
lexical-playground ❌ Failed (Inspect) Aug 31, 2022 at 7:03PM (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 Aug 31, 2022
@fantactuka

Copy link
Copy Markdown
Contributor

Do we need withProxy to accept callback rather than created node right away?

@trueadm

trueadm commented Aug 31, 2022

Copy link
Copy Markdown
Collaborator

Do we need withProxy to accept callback rather than created node right away?

How do you know what arguments get sent to the callback?

@fantactuka

fantactuka commented Aug 31, 2022 •

Copy link
Copy Markdown
Contributor

Do we need withProxy to accept callback rather than created node right away?

How do you know what arguments get sent to the callback?

Am I missing something? Return type will be taken from argument

$withNodeProxy($createTextNode(“hello”));

looking into withNodeProxy implementation and it just calls that great node callback, not doing anything special

@trueadm

trueadm commented Aug 31, 2022

Copy link
Copy Markdown
Collaborator

$withNodeProxy($createTextNode(“hello”));

Yeah that makes sense in the simple case. I thought about some of the more complex $createSomething constructors, and they sometimes involve statements over many lines. I guess it could accept both maybe?

@fantactuka

fantactuka commented Sep 1, 2022 •

Copy link
Copy Markdown
Contributor

Couple things to consider:

  • Do we keep importDOM on both nodes (original & proxied) or we ignore original node if there's a proxied
  • Introducing new node via Proxy over existing editor state (e.g. persisted on server): the next time editor is opened all old heading nodes recreated via Proxy and state is "updated". For some products it would mean user need to re-publish/re-save content

@YousefED

Copy link
Copy Markdown

Unfortunately, we can't make it automatic out of the box, $createTextNode will always create a TextNode and so will `new TextNode. However, for those pro users who require Proxies we can use a wrapper function that will facilitate this instead.

If I understand it correctly, this is severely limiting the usefulness of Proxies? Let's say you want to add some custom functionality to ParagraphNode, then you can only use Proxies if all plugins use $withNodeProxy instead of $createParagraphNode directly, right?

If we want to encourage always using $withNodeProxy or similar, should we stop exposing functions like $createParagraphNode?

@trueadm

trueadm commented Sep 29, 2022

Copy link
Copy Markdown
Collaborator

Unfortunately, we can't make it automatic out of the box, $createTextNode will always create a TextNode and so will `new TextNode. However, for those pro users who require Proxies we can use a wrapper function that will facilitate this instead.

If I understand it correctly, this is severely limiting the usefulness of Proxies? Let's say you want to add some custom functionality to ParagraphNode, then you can only use Proxies if all plugins use $withNodeProxy instead of $createParagraphNode directly, right?

If we want to encourage always using $withNodeProxy or similar, should we stop exposing functions like $createParagraphNode?

I don’t think so. The system is intentional in this respect, rather than magical. If I do $createTextNode then that’s all I want, I might not ever want someone to be able to proxy it. Systems that allow for magical monkey patching never scale well as it promotes tight coupling and unexpected runtime bugs that can crash the editor.

For the majority of use cases, people might be happy with using proxies however. So they should clearly state in the description of their plugin that they support proxies.

@GermanJablo

Copy link
Copy Markdown
Contributor

If I understand it correctly, this is severely limiting the usefulness of Proxies? Let's say you want to add some custom functionality to ParagraphNode, then you can only use Proxies if all plugins use $withNodeProxy instead of $createParagraphNode directly, right?

I could not agree more.

I don’t think so. The system is intentional in this respect, rather than magical. If I do $createTextNode then that’s all I want, I might not ever want someone to be able to proxy it. Systems that allow for magical monkey patching never scale well as it promotes tight coupling and unexpected runtime bugs that can crash the editor.

It seems to me that there is already too much explicit intent in using a proxy. I don't think even more layers of security should be added.

If I'm developing a plugin, I don't see any point in indicating in every part of the code if the nodes could be extended. It is something that should be evaluated by anyone thinking of extending the node.

On the other hand, I would say that in +90% of the cases users who want to extend a node will only want to add an attribute to it, and that hardly causes bugs.

@trueadm

trueadm commented Oct 14, 2022

Copy link
Copy Markdown
Collaborator

If I understand it correctly, this is severely limiting the usefulness of Proxies? Let's say you want to add some custom functionality to ParagraphNode, then you can only use Proxies if all plugins use $withNodeProxy instead of $createParagraphNode directly, right?

I could not agree more.

I don’t think so. The system is intentional in this respect, rather than magical. If I do $createTextNode then that’s all I want, I might not ever want someone to be able to proxy it. Systems that allow for magical monkey patching never scale well as it promotes tight coupling and unexpected runtime bugs that can crash the editor.

It seems to me that there is already too much explicit intent in using a proxy. I don't think even more layers of security should be added.

If I'm developing a plugin, I don't see any point in indicating in every part of the code if the nodes could be extended. It is something that should be evaluated by anyone thinking of extending the node.

On the other hand, I would say that in +90% of the cases users who want to extend a node will only want to add an attribute to it, and that hardly causes bugs.

From our experience - simply adding an attribute is dangerous and does cause significant issues, not to mention invaliding type safety and breaking collaboration assumptions. Just because it seems like the easy route, it doesn’t mean it’s the best route.

I honestly think we should proceed with proxies and evaluate them once people actually start using them, rather than making assumptions around their usefulness at a theoretical level.

@trueadm trueadm closed this Nov 21, 2022
@trueadm
trueadm deleted the factory branch November 24, 2022 12:17

This branch had an error being deployed

2 failed deployments
Preview – lexical-playground — 03a0fa0d Deployed Aug 31, 2022 by vercel[bot]
Preview – lexical — 03a0fa0d Deployed Aug 31, 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.

Decoupling plugin and nodes

6 participants