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 memory leaks due to onBroadcast and mutation errors. #7161

Merged

Conversation

kamilkisiela
Copy link
Contributor

Related #7160

@benjamn benjamn force-pushed the kamil-fix-mutation-error-leak branch from 74e22c2 to 067c2ef Compare October 15, 2020 21:21
@benjamn benjamn changed the base branch from main to release-3.3 October 15, 2020 21:21
@benjamn benjamn changed the title Fix mutation error leak Fix memory leaks due to onBroadcast and mutation errors. Oct 15, 2020
@benjamn benjamn added this to the Post 3.0 milestone Oct 15, 2020
@benjamn benjamn self-assigned this Oct 15, 2020
Copy link
Member

@benjamn benjamn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@kamilkisiela I agree with your analysis in #7160, but I decided to go a bit further by collapsing the MutationStore class into QueryManager. All of these changes are technically backwards compatible because QueryManager is a private API, but I decided to retarget this PR against release-3.3 just because the changes are more extensive/risky now.

Unless you object to my additional changes, I'll get this PR merged and release another AC3.3 beta shortly!

@benjamn benjamn force-pushed the kamil-fix-mutation-error-leak branch from 067c2ef to d53928c Compare October 15, 2020 21:51
@benjamn benjamn requested a review from jcreighton October 15, 2020 21:52
Comment on lines +251 to +257
onBroadcast: connectToDevTools ? () => {
if (this.devToolsHookCb) {
this.devToolsHookCb({
action: {},
state: {
queries: this.queryManager.getQueryStore(),
mutations: this.queryManager.mutationStore.getStore(),
mutations: this.queryManager.mutationStore || {},
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jcreighton Am I right to think this PR is backwards compatible with devtools, since this.queryManager.mutationStore is now the same as what this.queryManager.mutationStore.getStore() used to return?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes! This change won't be a big deal on the devtools side. 👍 Thanks for checking!

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants