yjs / y-prosemirror

ProseMirror editor binding for Yjs
https://demos.yjs.dev/prosemirror/prosemirror.html
MIT License
335 stars 116 forks source link

Introduce sub document support #88

Open DAlperin opened 2 years ago

DAlperin commented 2 years ago

This makes several key changes

DAlperin commented 2 years ago

@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 commented 2 years ago

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 commented 2 years ago

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 commented 1 year ago

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 commented 2 months ago

@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