-
Notifications
You must be signed in to change notification settings - Fork 23
Add onChange hook #119
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
base: main
Are you sure you want to change the base?
Add onChange hook #119
Conversation
- Adds an onOperation hook that intercepts all proxy paths - Very minimal performance impact
Thanks for your PR. From an implementation perspective, it's cleaner to add an extra change hook in |
Ah, interesting. I didn't notice this "all paths lead to markChanged" control point. Thank you. |
@unadlib What do you think about this latest version? Changes of note:
To accomplish the 2nd point, I needed to add additional
Then we could have a ternary at each call point: options.hooks?.onChange
? markChangedWithOnChange(target, key, { kind: 'set', prev: current, next: value })
: markChanged(target); I haven't benchmarked this as I'm not familiar with your best practices. |
I'm just re-reading your comment about using JSON Patch. I'm not sure if I understand this part of your reply, because the ProxyDraft passed to markChanged does not contain JSON Patch data afaict. EDIT: Ah, I see I missed |
In fact, we need to add new parameters to |
onOperationonChange hook that intercepts all proxy pathsI'm not sure if this addresses #72 , but I'm opening this PR for discussion since the onOperation hook as implemented by this PR addresses the sequence and interleaving issue I needed a solution to.
Disclosure: Used claude code, with personal review of code.