(fix) #641 unsubscribe Not Triggered with syncObservable#642
Conversation
17192e1 to
5f9fe4e
Compare
There was a problem hiding this comment.
Pull request overview
Fixes #641 by ensuring observables configured via syncObservable are recognized as “synced”, allowing listener-cleared propagation to trigger the underlying unsubscribe when the last observer is removed (including when only a nested property was observed).
Changes:
- Mark the target node as synced inside
syncObservableto enable correct cleanup/unsubscribe behavior. - Add
markNodeAsSyncedhelper inonChangeto centralize the synced-marking behavior. - Add a React test covering unmount → last observer removed → underlying unsubscribe called for
syncObservable.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
tests/react.test.tsx |
Adds a regression test ensuring syncObservable subscriptions unsubscribe after unmount when observing a nested property. |
src/sync/syncObservable.ts |
Marks the node as synced at setup time so listener-cleared events propagate as expected. |
src/onChange.ts |
Introduces markNodeAsSynced and simplifies synced detection typing to support the new behavior. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
You can also share your feedback on Copilot code review. Take the survey.
7debbdd to
ff06987
Compare
|
@DorianMazur I solved the test indentation that was requested in the review. Can you give it another try when you have a moment 🙏 |
|
@jmeistrich can you please take a look to this one when you have a moment 🙏 |
37a8e13 to
20daa95
Compare
20daa95 to
eaabd6f
Compare
|
Rebased, tweaked, and merged! I accidentally rebased one of my own fixes in here, sorry about that 😂 |
|
perfect, thanks! |
This PR solves #641
The synced observables created by
syncObservablewere not marked as synced, so the previous fix that I did for #621 was not working in this case.