Skip to content
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

[lexical] Feature: Exclude nodes from serialization #6364

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

TryingToImprove
Copy link
Contributor

@TryingToImprove TryingToImprove commented Jul 2, 2024

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

Copy link

vercel bot commented Jul 2, 2024

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

@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 Jul 2, 2024
Copy link

github-actions bot commented Jul 2, 2024

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%)

@TryingToImprove
Copy link
Contributor Author

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

@etrepum
Copy link
Contributor

etrepum commented Jul 2, 2024

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.

@TryingToImprove
Copy link
Contributor Author

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..

@etrepum
Copy link
Contributor

etrepum commented Jul 2, 2024

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

etrepum commented Jul 2, 2024

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.

@TryingToImprove
Copy link
Contributor Author

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..

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.

None yet

3 participants