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

Guard samples stale data from combined store #544

Closed
alpatovdanila opened this issue Oct 28, 2021 · 9 comments
Closed

Guard samples stale data from combined store #544

alpatovdanila opened this issue Oct 28, 2021 · 9 comments
Assignees
Labels
bug Something isn't working core effector package

Comments

@alpatovdanila
Copy link

What is the current behavior:
Guard passes stale data from combined store to effect.

Please provide the steps to reproduce and if possible a minimal demo of the problem via https://share.effector.dev, https://codesandbox.io or similar
https://share.effector.dev/jSQ3WPPg

What is the expected behavior:
I am expecting actual state of $requestData store to be passed to target effect

Which versions of effector packages, and which browser and OS are affected by this issue? Did this work in previous versions of effector?:
Bug occurs on 22.1.2 version of Effector

@alpatovdanila alpatovdanila added bug Something isn't working needs triage labels Oct 28, 2021
@zerobias zerobias added core effector package and removed needs triage labels Oct 28, 2021
@zerobias zerobias self-assigned this Oct 28, 2021
@zerobias
Copy link
Member

Agree, this should work in the same way

@MichaelShipitsyn
Copy link

It seems that the problem is not in guard. I think the problem is in calculation priority of the clock fn parameter in sample - https://share.effector.dev/MtzNQ2Gx

@zerobias
Copy link
Member

@MichaelShipitsyn yes, you right

The more I think about it, the more issues I find. The main question is whether the clock without source should have the same priority as the source in the usual case?

@AlexandrHoroshih
Copy link
Member

AlexandrHoroshih commented Oct 31, 2021

Weird enough, but wrapping clock in sample into another sample solves the issue 🤔
And making that sample wrapper greedy recreates the bug: https://share.effector.dev/mc4cQ01f

So isn't the problem actually in sample's clock batching mechanism? Looks like, in this situation, sample takes first triggered value of the clock instead of the last one

Another question i see is why there are two combine-ed store update calls?
First of those calls is the one with stale data, but i would expect only one call there 🤔

Changing that first sample to greedy one (here) or changing it to basic .on (there) fixes the issue because there is only one update of combined store - without any stale data 🤔

Doing the same change with the first reproduce also fixes the result: https://share.effector.dev/LWWbv7qt - no combine updates with stale data, no bug

zerobias added a commit that referenced this issue Nov 2, 2021
based on examples from @AlexandrHoroshih
related issue #544
zerobias added a commit that referenced this issue Nov 2, 2021
@zerobias
Copy link
Member

zerobias commented Nov 2, 2021

@AlexandrHoroshih big thanks for deep investigation, I found an issue using your examples 👍

Yes, the bug was in implementation of sample: in case with source store, the first value of clock was used instead of last one

I will publish a fix during this week

@zerobias
Copy link
Member

zerobias commented Nov 2, 2021

And as for multiple combine updates - it happens because $filterValues is updated strictly after $selectedCities → $requestData update chain. Look at ordering snapshots

@AlexandrHoroshih
Copy link
Member

AlexandrHoroshih commented Nov 2, 2021

Always glad to help 🤘

it happens because $filterValues is updated strictly after $selectedCities → $requestData update chain

Yes, i see that, but isn't this an observable glitchy value?

In those examples both dependencies of combined store are changed in the same computation cycle, so i would expect that combine will be updated (or at least call its watchers) only once during this cycle, without calling it on stale data 🤔

I see that this has something to do with sample priority level in this case, because by raising the priority manually (via greedy: true or using .on instead) combine triggered only once and we can no longer observe that stale value 🤔

@doasync
Copy link
Member

doasync commented Nov 11, 2021

I will publish a fix during this week

@zerobias How's it going with the fix?

@zerobias
Copy link
Member

@doasync I merged the release of this fix with a few others updates, will be released in an upcoming minor release as soon as I figure out the types for the sample

@zerobias zerobias added this to the effector 22.2.0 milestone Nov 24, 2021
@zerobias zerobias closed this as completed Dec 8, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working core effector package
Projects
None yet
Development

No branches or pull requests

5 participants