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

Regression PR 8826 broke cache.reset() #8851

Closed
tm1000 opened this issue Sep 27, 2021 · 5 comments · Fixed by #8852
Closed

Regression PR 8826 broke cache.reset() #8851

tm1000 opened this issue Sep 27, 2021 · 5 comments · Fixed by #8852

Comments

@tm1000
Copy link
Contributor

tm1000 commented Sep 27, 2021

Intended outcome:
Before #8826 was introduced we used cache.reset() to not only reset the cache but also recall all active query watchers. (much like client.resetStore()).

However, I noticed that #8826 (comment) seems to break this functionality

This seems to be a regression (Or were we using this wrong) as now the store is reset but no active queries are called again.

Actual outcome:
I'd Expect the active query watchers to be called as it worked before

How to reproduce the issue:
Call cache.reset() before and after #8826

@tm1000
Copy link
Contributor Author

tm1000 commented Sep 27, 2021

@benjamn if we were using cache.reset() wrong before let me know as we can just use client.resetStore() instead (it just seems strange to call the global client inside fo the client itself). Its just we were using cache.reset() inside of the update callback in some cases instead of having to manually rebuild caches that changes multiple fields.

benjamn added a commit that referenced this issue Sep 27, 2021
As reported in issue #8851, PR #8826 was a breaking change for code
expecting cache.watches to be restarted after cache.reset() is called.

This commit establishes a small but extensible Cache.ResetOptions API
for the cache.reset method, which allows the handling of watches to be
configured via options.discardWatches, which defaults to false, thus
restoring the behavior of cache.reset() from before #8826.

If you need the behavior introduced by #8826, you can now opt (back)
into that behavior by calling cache.reset({ discardWatches: true }).
@benjamn
Copy link
Member

benjamn commented Sep 27, 2021

Thanks for the quick feedback @tm1000!

I see now how calling this.broadcastWatches() after this.init() was a reliable way for cache.reset to rewatch all watchers (that is, before #8826), so I agree this was a breaking change.

In 3.4.15, we will restore the pre-3.4.14 behavior by default, while supporting

cache.reset({ discardWatches: true })

as a way to opt into the behavior of #8826.

Does that meet your needs/expectations @tm1000? Happy to reopen if not!

@tm1000
Copy link
Contributor Author

tm1000 commented Sep 27, 2021

@benjamn Thanks for the quick response. I think it fixes the issues but my engineers have told me that cache.reset() actually broke between 3.3.21 and 3.4.0 which was rather unexpected (they first told me it was related to this and then they ran through downgrading until they got to 3.3.21 which they said worked)

Looking at the code though it looks like the watchers should refetch even in 3.4.0 and especially after your work in #8826 so the investigation continues but without the work in #8826 it definitely wouldn't have worked at all.

Either way this specific issue is resolved. If I find something else I'll open a new issue in regards to that but again #8826 was still needed because it brings back watchers being able to refetch at least

@benjamn
Copy link
Member

benjamn commented Sep 27, 2021

Alas, 3.3.21 was the last version before 3.4.0, which was a relatively big minor release. Of all the changes in 3.4.0, 526c940 (something I added to #8107, perhaps unwisely) looks the most :suspect: to me.

Please do open a new issue if you find anything more. Now that we have the Cache.ResetOptions interface, we can add new options to cache.reset relatively easily, to accommodate more use cases simultaneously.

@tm1000
Copy link
Contributor Author

tm1000 commented Sep 27, 2021

@benjamn Thanks for pointing me in the right direction!

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Feb 1, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.