facebook / lexical

Lexical is an extensible text editor framework that provides excellent reliability, accessibility and performance.
https://lexical.dev
MIT License
17.5k stars 1.45k forks source link

Allow overriding HTML serialization behavior from the editor config. #4254

Closed acywatson closed 8 months ago

acywatson commented 1 year ago

With this change, I'm adding a new path for changing or extending html serialization functionality in the editor. Currently, if you want to change HTML serialization behavior on the core nodes, you generally need to override the node, which I think is an unnecessarily confusing process, given how common the use case is.

    html: {
      export: new Map([[TextNode, () => ({ element: document.createElement('figure') })]]),
      import: { 'figure': () => ({ conversion: () => ({ node: $createTextNode('yolo') }), priority: 4 }) }
    },
vercel[bot] commented 1 year ago

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 Sep 14, 2023 4:04pm
lexical-playground ✅ Ready (Inspect) Visit Preview 💬 Add feedback Sep 14, 2023 4:04pm
github-actions[bot] commented 1 year ago

size-limit report 📦

Path Size Loading time (3g) Running time (snapdragon) Total time
packages/lexical/dist/Lexical.js 28.27 KB (0%) 566 ms (0%) 46 ms (+16.22% 🔺) 611 ms
packages/lexical-rich-text/dist/LexicalRichText.js 39.55 KB (+0.19% 🔺) 791 ms (+0.19% 🔺) 68 ms (+61.4% 🔺) 859 ms
packages/lexical-plain-text/dist/LexicalPlainText.js 39.53 KB (+0.19% 🔺) 791 ms (+0.19% 🔺) 43 ms (+11.56% 🔺) 834 ms
acywatson commented 1 year ago

Can you elaborate on why we should prefer this over replacement classes? The proposed appraoch diverges from the one way to do things approach and can likely lead into more single individual methods being ovewritten in a similar fashion.

@zurfyx

Yea, I touched on it in the PR description, but if you look at the feedback and questions we have gotten/are getting around HTML serialization, we have a proliferation of issues around minor tweaks people want to do to how core nodes are imported/exported to/from HTML.

So, to cover the simple use case of adding support for backgroundColor (for instance) in in HTML serialization, you need to 1) learn about node overrides 2) create your node class (which means you need to learn about getType and clone, etc), then 3) figure out how to change export/importDOM on that node to do what you want. importDOM in particular is not easy to extend - you'd typically need to copy over the logic to a new conversion to add support for a single property.

the one way to do things approach and can likely lead into more single individual methods being ovewritten in a similar fashion.

I'm not sure that this is really a problem in this context. Can you tell me the practical problem you would see with their being a simpler way for basic extension and a more flexible way for more advanced use cases?

zurfyx commented 1 year ago

I'm not sure that this is really a problem in this context. Can you tell me the practical problem you would see with their being a simpler way for basic extension and a more flexible way for more advanced use cases?

I see what you mean, my first intuition would be to evaluate whether it's possible to build a higher-level API on top of node replacement. Type-safety might not make it easy though.

Second, but that's way beyond this PR, Nodes require heavy configuration, we should simplify them. A 5-liner should be sufficient to get you started with your own Node.

Third, for TextNode and ElementNode which are the most flexible and also built-in into the Core we could consider features where users can seamlessly toggle what they need. As basic Nodes are more and more powerful we will encounter more and more cases of users only wanting a subset of features.

That said, I'm pretty sure you've thought more about this space than I have so I've no objections towards merging this, can we get some thoughts from other folks too? @fantactuka @thegreatercurve @tylerjbainbridge

acywatson commented 1 year ago

Second, but that's way beyond this PR, Nodes require heavy configuration, we should simplify them. A 5-liner should be sufficient to get you started with your own Node.

I particularly like this

I was thinking that reviving and merging @EgonBolton's clone and serialization PRs would be a big step in the right direction.

acywatson commented 1 year ago

Third, for TextNode and ElementNode which are the most flexible and also built-in into the Core we could consider features where users can seamlessly toggle what they need. As basic Nodes are more and more powerful we will encounter more and more cases of users only wanting a subset of features.

With this one, I wonder what it would mean to have "features" toggled on or off. I thought about this a bit, but it's hard to rationalize what it would mean to turn something "on" or "off" when there are so many other ways to control that thing through other APIs.

acywatson commented 1 year ago

I see what you mean, my first intuition would be to evaluate whether it's possible to build a higher-level API on top of node replacement. Type-safety might not make it easy though.

Interesting - what could this look like?

zurfyx commented 1 year ago

With this one, I wonder what it would mean to have "features" toggled on or off. I thought about this a bit, but it's hard to rationalize what it would mean to turn something "on" or "off" when there are so many other ways to control that thing through other APIs.

I don't think this addresses the entire problem but it can work for a subset. For example, you can define a TextPlugin

<TextPlugin hasStrikethrough={false} /> or <TablePlugin hasCellMerging={true} />

The plugin will prevent and "normalize" bad features. We can even go further and build it into the Node with some invariants but I'm hesitant on introducing additional layer of inheritance (this is one of the flaws of the prototype model).

Interesting - what could this look like?

Probably not a good idea. I don't think it's wise to hide the implementation details behind the class. Not only they should be well aware of its existance but also they will need a reference to it and a $isXNode function. In this case, I'd double down on the idea of simplifying node creation and/or codegen Nodes (since most of it is copy-paste anyway).

acywatson commented 1 year ago

The plugin will prevent and "normalize" bad features.

Via transforms, I guess? How would you implement this?

zurfyx commented 1 year ago

Via transforms, I guess? How would you implement this?

Yes, that's what I had in mind, even though there might be some UX trade-offs at points.

abelsj60 commented 8 months ago

Via transforms, I guess? How would you implement this?

Yes, that's what I had in mind, even though there might be some UX trade-offs at points.

This is just a small comment:

  1. This is a very interesting conversation.
    • @acywatson, I like the way you're thinking here. I just answered a question on Discord where the tediousness of having to change every single core node made one idea seem "correct," but exhausting. For what that's worth.
  2. This reminds me of that schema discussion that was had at some point. Whenever you get around to focusing on these ideas more, maybe that conversation informs this one? Or vice versa? Just a thought.
GermanJablo commented 8 months ago

Second, but that's way beyond this PR, Nodes require heavy configuration, we should simplify them. A 5-liner should be sufficient to get you started with your own Node.

I particularly like this

I was thinking that reviving and merging @GermanJablo's clone and serialization PRs would be a big step in the right direct

Thanks for pointing this out. By the way, what's the status of this?

I see what you mean, my first intuition would be to evaluate whether it's possible to build a higher-level API on top of node replacement. Type-safety might not make it easy though. ... Third, for TextNode and ElementNode which are the most flexible and also built-in into the Core we could consider features where users can seamlessly toggle what they need. As basic Nodes are more and more powerful we will encounter more and more cases of users only wanting a subset of features.

I'm going to address points 1 and 3 here.

The problem with making features optional is that it increases the surface area of the API and does not solve the problem that customizing something becomes difficult or impossible.

After thinking about it a lot, I've only found two ways to flexibly extend the behavior of a node: mixins and prototypes.

Prototypes: Extending the prototype basically consists of doing what @fantactuka suggested in this comment. I've tested it on small examples and it seemed to work. I think it might work well with Typescript if declare global is used, But I don't know how composable it is (in terms of adding more of a "component" or "decorator" functionality to a node).

Mixins: The second alternative, which is what I'm using, is to use mixins. It took me a long time to make it typesafe, but I finally did, even making type guards of the type $isNodeX (or rather $hasXMixin), which is great. I showed an example on Discord.

The only problem with mixins so far is that static methods have to be repeated. My PRs removing clone and importJSON would make this easier. In the example I gave I also showed how to override getType.

Indent and format are two use cases that, for example, would bring many benefits to be implemented with this pattern. That is what I wanted to explain in this issue: https://github.com/facebook/lexical/issues/4528

acywatson commented 8 months ago

Thanks for pointing this out. By the way, what's the status of this?

This will be the next thing that I work on in Lexical. I was going to checkout your PR and fix all those conflicts for you because I feel so bad about the delay. Then I think we are good to merge.

Mixins: The second alternative, which is what I'm using, is to use mixins. It took me a long time to make it typesafe, but I finally did, even making type guards of the type $isNodeX (or rather $hasXMixin), which is great. I showed an example on Discord.

This is cool...then we could vend mixins for things like this. You'd still have to use node overrides API though, I suppose.

GermanJablo commented 8 months ago

This will be the next thing that I work on in Lexical. I was going to checkout your PR and fix all those conflicts for you because I feel so bad about the delay. Then I think we are good to merge.

haha thanks. If I find time I can update it, although I don't think I can this week.

This is cool...then we could vend mixins for things like this. You'd still have to use node overrides API though, I suppose.

Exactly. The nodes on which you apply the mixin are the ones that you register in the editor, using the replacement API.

acywatson commented 8 months ago

This will be the next thing that I work on in Lexical. I was going to checkout your PR and fix all those conflicts for you because I feel so bad about the delay. Then I think we are good to merge.

haha thanks. If I find time I can update it, although I don't think I can this week.

This is cool...then we could vend mixins for things like this. You'd still have to use node overrides API though, I suppose.

Exactly. The nodes on which you apply the mixin are the ones that you register in the editor, using the replacement API.

I'm gonna merge this, but I really like the mixin idea - let me think about how best to carry that forward.

moy2010 commented 5 months ago

Just gave it a try on Lexical 0.12.4 by passing an initialConfig to a LexicalComposer, and didn't see any transformation on the HTML content.

Is this feature live now?