-
-
Notifications
You must be signed in to change notification settings - Fork 239
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
fix(utils): make derive glitch free #335
Conversation
This pull request is being automatically deployed with Vercel (learn more). 🔍 Inspect: https://vercel.com/pmndrs/valtio/o8ctRVtBQgUTvNvw8gi8cSGB8xC8 |
This pull request is automatically built and testable in CodeSandbox. To see build info of the built libraries, click here or the icon next to each commit SHA. Latest deployment of this branch, based on commit e3a37a0:
|
Size Change: +716 B (+2%) Total Size: 33.4 kB
ℹ️ View Unchanged
|
@@ -211,7 +211,7 @@ it('nested emulation with derive', async () => { | |||
{ | |||
doubled: (get) => computeDouble(get(state.math).count), | |||
}, | |||
{ proxy: state.math } | |||
{ proxy: state.math, sync: true } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is technically a behavioral change, but hope it's not too critical.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So if it's async it can't be glitch free?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, but I didn't confirm it in the nested scenario. Without sync
in this case, it simply double call the subscriber. It's a side effect of supporting glitch free.
…341) * fix(tests): make glitch test count right * tweak a test * fix: derive with shared subscriptions * refactor * revise comments Co-authored-by: daishi <daishi@axlight.com>
Just tested it, wow it's so awesome! Even when |
Thanks for confirming. That's great. |
Thank you brother! |
@mweststrate hinted that a solution to make glitch free is simply to use a counter. https://twitter.com/mweststrate/status/1484928361084379136
So I tried. closes #296
I had to make the subscription sync and then async in derive. There might be some edge cases this won't work well, like mixing sync and async, but the basic patterns should be covered. Let's see how it goes with releasing this.