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

Fix stale computed values #300

Merged
merged 5 commits into from
May 5, 2024
Merged

Conversation

russelldavis
Copy link

Fixes #299

This fixes stale values in computed stores by checking if dependencies have changed any time get() is called. It does have a perf impact, though I added an epoch optimization to help with that. And correctness should take precedence over performance (no other state library I'm aware of has this stale value problem). The benefits are:

  • computed stores never return stale values
  • this also solves the diamond problem, so the previous logic that was added for that is no longer needed, which means:
    • simpler logic
    • no need to store and compute a level (l) for each atom
    • bundle size reduced (min: 286B to 235B, max: 818B to 769B)
    • better perf in certain areas -- processing the listenerQueue is now O(n) instead of O(n^2)

Another potential benefit -- this change means that when calling get() on an atom with 0 subscribers, we no longer have to add a dummy subscriber. I'm holding off on making that change, because it would be a breaking change -- some code could be relying on the side effect of onMount firing. But it could be considered for a future major version -- IMO it would be cleaner. Right now, if you call get() on a computed store, it will never unsubscribe, and the value will be needlessly recomputed when the dependencies change, even if the only listener is the dummy listener.

The previous commit's solution to the stale atom problem also solves the
diamond problem without needing special handling of atom levels.
@ai
Copy link
Member

ai commented May 3, 2024

this change means that when calling get() on an atom with 0 subscribers, we no longer have to add a dummy subscriber

Is It possible to return this behaviour?

I use it a lot on my CRDT stores. The store value before onMount is just undefined.

Right now, if you call get() on a computed store, it will never unsubscribe

Seems like it is a bug. The dummy subscriber for listener-less get() should be removed after.

@russelldavis
Copy link
Author

Is It possible to return this behaviour?

I didn't change the behavior in this PR since it would be a breaking change. It's just a suggestion for a possible future change that would be possible after this PR. We can ignore it for now and discuss it as a separate issue if this PR gets merged.

@ai ai changed the base branch from main to next May 5, 2024 13:12
@ai
Copy link
Member

ai commented May 5, 2024

In this case everything looks amazing. I will merge it to next and release in next major release (I am afraid on invisible breaking changes).

@ai ai merged commit f1e5184 into nanostores:next May 5, 2024
@ai
Copy link
Member

ai commented May 5, 2024

I merge to next branch and it broke the tests :(

Can you find the reason? https://github.com/nanostores/nanostores/actions/runs/8958545657/job/24602887313

russelldavis added a commit to russelldavis/nanostores that referenced this pull request May 5, 2024
The new code in #380 needed to be updated to reflect the new lower
number of items in listenerQueue per listener.
@russelldavis
Copy link
Author

Fixed at #301

ai pushed a commit that referenced this pull request May 5, 2024
The new code in #380 needed to be updated to reflect the new lower
number of items in listenerQueue per listener.
ai added a commit that referenced this pull request Jun 25, 2024
* Prevent stale computed values

* Remove atom levels

The previous commit's solution to the stale atom problem also solves the
diamond problem without needing special handling of atom levels.

* Optimize computed dependency check using epochs

* Update size-limit

---------

Co-authored-by: Andrey Sitnik <andrey@sitnik.ru>
ai pushed a commit that referenced this pull request Jun 25, 2024
The new code in #380 needed to be updated to reflect the new lower
number of items in listenerQueue per listener.
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.

computed store can return stale value
2 participants