lexical icon indicating copy to clipboard operation
lexical copied to clipboard

[lexical] Feature: Exclude nodes from serialization

Open TryingToImprove opened this issue 1 year ago • 8 comments

This tries to solve #4273

My use case for this is that I am adding a DecoratorNode into my editor, however I do not want to persist that Node.

I would like feedback, since I am new to the codebase

TryingToImprove avatar Jul 02 '24 06:07 TryingToImprove

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

Name Status Preview Comments Updated (UTC)
lexical ❌ Failed (Inspect) Jul 2, 2024 7:06am
lexical-playground ✅ Ready (Inspect) Visit Preview 💬 Add feedback Jul 2, 2024 7:06am

vercel[bot] avatar Jul 02 '24 06:07 vercel[bot]

size-limit report 📦

Path Size
lexical - cjs 28.47 KB (0%)
lexical - esm 28.28 KB (0%)
@lexical/rich-text - cjs 36.86 KB (0%)
@lexical/rich-text - esm 28.08 KB (0%)
@lexical/plain-text - cjs 35.49 KB (0%)
@lexical/plain-text - esm 25.3 KB (0%)
@lexical/react - cjs 38.82 KB (0%)
@lexical/react - esm 29.27 KB (0%)

github-actions[bot] avatar Jul 02 '24 06:07 github-actions[bot]

I will need some guidance on whether or not this would be the correct way to handle excluding nodes before I continue :)

TryingToImprove avatar Jul 02 '24 08:07 TryingToImprove

I think the excludeFromCopy approach discussed in #4273 would be easier to implement correctly and have fewer consequences for existing code (maybe using the clone destination? I don't see any existing code in the monorepo that explicitly deals with that, but it's already present on the type). Changing the return type of exportJSON is very much not backwards compatible.

etrepum avatar Jul 02 '24 15:07 etrepum

I think the excludeFromCopy approach discussed in #4273 would be easier to implement correctly and have fewer consequences for existing code (maybe using the clone destination? I don't see any existing code in the monorepo that explicitly deals with that, but it's already present on the type). Changing the return type of exportJSON is very much not backwards compatible.

Yea, I was also considering that approach, however I would be surprised myself if I did an serialization and did not get the node because of excludeFromCopy, but yes might seem like the best approach..

As far as I can see the clone is not being used for anything, but would probably prefer an destination like serialization

The backward compatibility issue is one of the reason I stopped where there code is now, since a lot of stuff breaks if exportJSON can return null too.

The reason I went with returning null was to be aligned with exportDOM..

TryingToImprove avatar Jul 02 '24 15:07 TryingToImprove

I think there are three reasonable approaches:

  1. Use excludeFromCopy with the existing clone destination. This will change semantics for nodes that implement this method without considering the destination.
  2. Use excludeFromCopy with a new destination. This will change semantics for nodes that implement this method without considering the destination, and may require code changes for compatibility with the new parameter type.
  3. Add a new method specifically for this purpose (e.g. excludeFromExport), possibly with the intent of being more general (e.g. it could defer to the existing excludeFromCopy in some situations). This would be fully backwards compatible and not change semantics for any existing code (unless there is a name conflict), but it would make the API more complex.

etrepum avatar Jul 02 '24 16:07 etrepum

With a quick look at the monorepo code it seems that options 1 & 2 would probably break the current implementation of OverflowNode, since it unconditionally returns true from excludeFromCopy. Wouldn't necessarily be a problem on its own since it could be updated at the same time, but if others have copied its implementation and expect it to be JSON serialized then that could cause problems.

etrepum avatar Jul 02 '24 16:07 etrepum

I think I prefer excludeFromExport with an destination parameter like excludeFromCopy so it would be possible to use it for both HTML and JSON..

The excluding of nodes can be done in user-land as I have done it now and the issue is kind of stale, so the feature might not be worth the complexity - I am not sure..

TryingToImprove avatar Jul 03 '24 06:07 TryingToImprove

Closing this PR due to staleness! If there are new updates, please reopen the PR.

etrepum avatar Dec 02 '24 19:12 etrepum