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

Introduce sub document support #88

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

DAlperin
Copy link

@DAlperin DAlperin commented Jan 1, 2022

This makes several key changes

  • introduces a new optional "selfID" param in the cursor-plugin constructor - this was introduced to mitigate an edge case where there are multiple documents using the same stateField which could lead to the plugin trying to render it's own cursor events.
  • respects the stateField everywhere
  • Add some sanity checking to prevent footguns with subdocuments such as making sure that the plugin only responds to events coming from a document with the same guid

parent 984175f
author Dov Alperin <[email protected]> 1635443219 -0400
committer Dov Alperin <[email protected]> 1640994693 -0500

Make y-prosemirror behave with subdocuments

+ throw some safety checks in to avoid common (in my testing) footguns

Check for object nullness in equalAttrs

As typeof null is object, equalAttrs falsely expects it to produce
Object.keys thus crashing when comparing object against a null value.

update lib0

1.0.11 (upstream)

Linking to documentation for Awareness protocol.

Some refactors

Bump with bugfix

Bump again

Adopt yjs#71 h/t @hoebbelsB

Revert "Adopt yjs#71 h/t @hoebbelsB"

This reverts commit 0039353.

Use our own protocols

Who needs safety checks anyway?

Revert some debugging changes in search of a known good state

Experimental fix for the cursor jumping bug

More hacks for subdocument bs

revert

parent 984175f
author Dov Alperin <[email protected]> 1635443219 -0400
committer Dov Alperin <[email protected]> 1640994693 -0500

Make y-prosemirror behave with subdocuments

+ throw some safety checks in to avoid common (in my testing) footguns

Check for object nullness in equalAttrs

As typeof null is object, equalAttrs falsely expects it to produce
Object.keys thus crashing when comparing object against a null value.

update lib0

1.0.11 (upstream)

Linking to documentation for Awareness protocol.

Some refactors

Bump with bugfix

Bump again

Adopt yjs#71 h/t @hoebbelsB

Revert "Adopt yjs#71 h/t @hoebbelsB"

This reverts commit 0039353.

Use our own protocols

Who needs safety checks anyway?

Revert some debugging changes in search of a known good state

Experimental fix for the cursor jumping bug

More hacks for subdocument bs

revert

can pass undoManager to undo-plugin

can pass undoManager to undo-plugin

can pass undoManager to undo-plugin

can pass undoManager to undo-plugin

remove yarn.lock

1.0.14

Bump

Respect stateField key everywhere actually

Fix some places that ignored the statekey
@DAlperin
Copy link
Author

DAlperin commented Jan 1, 2022

@dmonad this is hopefully the first in a series of pull requests making sub documents a first class citizen across the yjs ecosystem.

The next one I have in mind is against y-websocket which introduces the concept of sub documents to the binary protocol.

If you are ok with my protocol implementation I would love to try and get it into more providers as well as the yjs documentation

@DAlperin
Copy link
Author

DAlperin commented Jan 1, 2022

Github seems to think I have touched src/plugins/undo-plugin.js but as far as I can tell my branch seems to be consistent with master (to be clear, none of my work has touched the undo plugin)? github might be comparing that file against an old commit.

@DAlperin
Copy link
Author

DAlperin commented Jan 5, 2022

I've been documenting this internally so I wanted to bring some of the notes into here

Add some sanity checking to prevent footguns with subdocuments such as making sure that the plugin only responds to events coming from a document with the same guid

introduces a new optional "selfID" param in the cursor-plugin constructor - this was introduced to mitigate an edge case where there are multiple documents using the same stateField which could lead to the plugin trying to render it's own cursor events.

Neither of these are strictly necessary, but they were very helpful in porting non subdoc yjs code over without completely breaking everything right away. The specific problem that prompted these changes was (IIRC) when using the patched y-prosemirror inside another piece of software with it's own assumptions about the default stateKey, in particular I remember being initially extremely confused trying to set up/port tiptap support because of this (tiptap still does some complaining but that will be fixed in some patches I am hoping to upstream to prosemirror itself). In the future I think we would probably be better served throwing an error if a client tries to subscribe a document to a statekey another document is already using, but until subdocuments become more standard I am worried that will unnecessarily break things for people with niche use cases.

I played around with some more attempts at automated error correction for more accidental footguns I ran into but I think the complexity and runtime overhead isn't worth. Though I do also imagine as server supplementations standardize on the "right" way to handle subdocs a lot of those problems will disappear over time.

respects the stateField everywhere

I am maintaining some internal docs on "subdocument best practices" such as assigning each their own stateKey and I will open a PR against the docs once we get it all sorted out.

@abrgr
Copy link

abrgr commented May 11, 2023

Thanks so much for the amazing yjs @dmonad!

And thanks for putting this together @DAlperin.

I'm happy to help with anything needed to get this landed if this is a direction you would be open to, Kevin. Consistent support for cursorStateField would be super useful.

@andrictham
Copy link

andrictham commented Jul 10, 2024

@DAlperin @dmonad I know this is pretty stale, but is there any other reason that this wasn’t merged?

I’m building with subdocuments and currently running into the issue where cursors are only respecting cursorStateField when writing into awareness state, but not when reading from it to create decorations.

I’m only running into this issue because cursors don’t support subdocuments in the first place: all subdocs read and write cursor state from the same cursor namespace, and I was looking for a way to namespace them so each subdocument would sync its own cursor state.

Regardless of subdocument support, the fact that cursorStateField properly namespaces cursor state when setting state, but not when reading back from it seems like a bug (first reported in #86).

I don’t know if this is truly a bug or intended behaviour (I couldn’t find any documentation about how cursorStateField is meant to be used). Would appreciate guidance around this @dmonad

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants