joebobmiles / zustand-middleware-yjs

Zustand middleware that enables sharing of state between clients via Yjs.
MIT License
103 stars 10 forks source link

feat: allow developers to pass a transaction origin to the middleware #59

Closed walfly closed 2 months ago

walfly commented 3 months ago

I didn't see a contributor readme anywhere, so let me know if there are any changes you would like me to make to this pr.

The goal of this pr is to allow developers to pass an origin value to the middleware so that they can distinguish updates coming from the store in any event handlers they have set up.

joebobmiles commented 3 months ago

I'll need to write contributor guidelines. The only issues I see are the commit message format (I use an automated release pipeline that requires Conventional Commit syntax) and merging to master rather than staging.

That being said, I'm unsure of the utility behind this feature. Could you tell me what use case you've run into that needs to identify the source of an update to a document? Is it to identify which peer sent an update? And if so, why?

walfly commented 3 months ago

I'm using it to track changes I made as opposed to my peers. I have a page with a version history timeline similar to what you would see on a google doc if you click the clock icon. This history is created similar to the y-prosemirror-versions example. With some added logic to make the versions happen automatically and separate the updates by client. Because it's all decentralized, I need to filter a list of updates for only those that i'm responsible for. Hence adding an origin to each client store is very helpful. Previously i was tracking everything besides the zustand store and then filtering them out, but that approach becomes unmaintainable as the event sources in my application grows.

It's also very useful in debugging to be able to track down which piece of the application is firing updates when.

walfly commented 3 months ago

Commit message updated

walfly commented 3 months ago

@joebobmiles What's the verdict here, will this get merged or should i use a forked version?

joebobmiles commented 3 months ago

Sorry for the delay. I think there are interesting applications for making use of transaction origins, particularly as I read the Yjs docs. I'm tempted to utilize them to help with managing the two-way binding behavior, particularly when it comes to the broken Immer compatibility (#53).

However, that puts this change in a tough spot. I can understand the benefit to the debugging experience, but would you get that same value if the origin was hardcoded in the middleware?

The reason I ask is that an alternative to this is solving the Immer compatibility by introducing a middleware-specific transaction origin that prevented Yjs from bouncing changes back to Zustand. You'd get a transaction origin identifying any changes as coming from the middleware, but you wouldn't get any additional information other than what the middleware provides in the origin object. Part of the solution could also be to allow developers to include extra data in the origin object, but I don't know if that would provide more value over some simple metadata that says "hey, this change came from zustand-middleware-yjs."

walfly commented 3 months ago

I probably can make what i want work with a static origin name. The tiptap yjs plugin has a default one but also lets you override it . Alternatively, the type of origin is any in yjs. So we could just do both and make origin something like.

type Origin = {
  defaultName: string;
  customName?: string;
}