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

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

Closed
wants to merge 4 commits into from

Conversation

walfly
Copy link

@walfly walfly commented Apr 28, 2024

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.

fix: include "types" in exports field
@joebobmiles joebobmiles changed the base branch from master to staging April 28, 2024 19:11
@joebobmiles
Copy link
Owner

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?

@joebobmiles joebobmiles self-requested a review April 28, 2024 19:20
@walfly
Copy link
Author

walfly commented Apr 28, 2024

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.

… so that event handlers can detect where updates come from
@walfly
Copy link
Author

walfly commented Apr 28, 2024

Commit message updated

@walfly walfly changed the title Allow developers to pass a transaction origin to the middleware feat: allow developers to pass a transaction origin to the middleware Apr 28, 2024
@walfly
Copy link
Author

walfly commented May 3, 2024

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

@joebobmiles
Copy link
Owner

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

walfly commented May 25, 2024

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;
}

@walfly walfly closed this Jun 3, 2024
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.

None yet

2 participants