-
Notifications
You must be signed in to change notification settings - Fork 1.7k
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
Allow overriding HTML serialization behavior from the editor config. #4254
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
size-limit report 📦
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
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
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 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 |
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. |
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. |
Interesting - what could this look like? |
I don't think this addresses the entire problem but it can work for a subset. For example, you can define a TextPlugin
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).
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 |
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. |
dcdff5a
to
31c5abf
Compare
This is just a small comment:
|
Thanks for pointing this out. By the way, what's the status of this?
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 The only problem with mixins so far is that static methods have to be repeated. My PRs removing
|
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.
This is cool...then we could vend mixins for things like this. You'd still have to use node overrides API though, I suppose. |
haha thanks. If I find time I can update it, although I don't think I can this week.
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. |
17de3f6
to
a1e130f
Compare
Just gave it a try on Lexical 0.12.4 by passing an initialConfig to a Is this feature live now? |
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.