Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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: add a generic state property to all nodes #7117
base: main
Are you sure you want to change the base?
[lexical] Feature: add a generic state property to all nodes #7117
Changes from 9 commits
a6288f9
a90a719
448e2af
bf70110
6ebd4e5
e46d735
e404d32
c1cfce4
6d00c64
e0c5945
7728780
d807d4f
90a032b
c663540
113a4f1
91148f3
47435b0
fb7e164
eab152e
d2b4f21
2bc4a0e
fca2071
8cf6855
abd3d3e
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
Perhaps I could have used Object.defineProperty to avoid having to modify the tests, but I preferred to do this because it turned out that there were few tests affected and the changes were trivial.
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.
I think this will probably be necessary to implement for correct yjs behavior
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.
I already followed your suggestion to copy the state object in setState, so this suggestion is no longer necessary?
On the other hand, do you know what a unit test for yjs could look like?
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.
I am not familiar with the intricacies of testing lexical-yjs off-hand, I just know its implementation is very problematic because it manipulates node properties directly rather than going through the constructor and well defined serializations. I'd find another node that uses a non-primitive property (something to do with comments or marks probably?) and see if there's any relevant testing. There might not be any good testing for this sort of thing currently.
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.
I tried to see if collision checking could be done at runtime, but I came to the conclusion that it's not a good idea.
bg-color
would probably do the same thing.In any case, the recommendation here in the documentation would still be valid.
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.
A map+object API would allow this kind of check on first use although to make this efficient you’d use Map<string, StateKey> and have the object be underlying storage (or have a separate object for parsed values)
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.
With a single object implementation this must always call parse because we don’t know if its directly from JSON or not