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.

Sign up for GitHub

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
merged 5 commits into from
Oct 16, 2020
+72 −104
Merged

Fix memory leaks due to onBroadcast and mutation errors. #7161

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
9705f66
Fix memory leaks due to onBroadcast and mutation errors.
kamilkisiela Oct 14, 2020
83d0b0a
Simplify connectToDevTools logic in ApolloClient constructor.
benjamn Oct 15, 2020
1f60b92
Allow queryManager.onBroadcast to remain undefined.
benjamn Oct 15, 2020
ad4be6e
Avoid creating queryManager.mutationStore when unused.
benjamn Oct 15, 2020
d53928c
Update tests after restructuring queryManager.mutationStore.
benjamn Oct 15, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
38 changes: 20 additions & 18 deletions src/__tests__/optimistic.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -147,6 +147,8 @@ describe('optimistic mutation results', () => {
return null;
},
}),
// Enable client.queryManager.mutationStore tracking.
connectToDevTools: true,
});

const obsHandle = client.watchQuery({ query });
Expand Down Expand Up @@ -377,9 +379,9 @@ describe('optimistic mutation results', () => {
);

// @ts-ignore
const latestState = queryManager.mutationStore;
expect(latestState.get('1').loading).toBe(false);
expect(latestState.get('2').loading).toBe(true);
const latestState = queryManager.mutationStore!;
expect(latestState[1].loading).toBe(false);
expect(latestState[2].loading).toBe(true);

return res;
});
Expand All @@ -397,17 +399,17 @@ describe('optimistic mutation results', () => {
);

// @ts-ignore
const latestState = queryManager.mutationStore;
expect(latestState.get('1').loading).toBe(false);
expect(latestState.get('2').loading).toBe(false);
const latestState = queryManager.mutationStore!;
expect(latestState[1].loading).toBe(false);
expect(latestState[2].loading).toBe(false);

return res;
});

// @ts-ignore
const mutationsState = queryManager.mutationStore;
expect(mutationsState.get('1').loading).toBe(true);
expect(mutationsState.get('2').loading).toBe(true);
const mutationsState = queryManager.mutationStore!;
expect(mutationsState[1].loading).toBe(true);
expect(mutationsState[2].loading).toBe(true);

checkBothMutationsAreApplied(
'Optimistically generated',
Expand Down Expand Up @@ -621,9 +623,9 @@ describe('optimistic mutation results', () => {
);

// @ts-ignore
const latestState = client.queryManager.mutationStore;
expect(latestState.get('1').loading).toBe(false);
expect(latestState.get('2').loading).toBe(true);
const latestState = client.queryManager.mutationStore!;
expect(latestState[1].loading).toBe(false);
expect(latestState[2].loading).toBe(true);

return res;
});
Expand All @@ -641,17 +643,17 @@ describe('optimistic mutation results', () => {
);

// @ts-ignore
const latestState = client.queryManager.mutationStore;
expect(latestState.get('1').loading).toBe(false);
expect(latestState.get('2').loading).toBe(false);
const latestState = client.queryManager.mutationStore!;
expect(latestState[1].loading).toBe(false);
expect(latestState[2].loading).toBe(false);

return res;
});

// @ts-ignore
const mutationsState = client.queryManager.mutationStore;
expect(mutationsState.get('1').loading).toBe(true);
expect(mutationsState.get('2').loading).toBe(true);
const mutationsState = client.queryManager.mutationStore!;
expect(mutationsState[1].loading).toBe(true);
expect(mutationsState[2].loading).toBe(true);

checkBothMutationsAreApplied(
'Optimistically generated',
Expand Down
27 changes: 11 additions & 16 deletions src/core/ApolloClient.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -140,7 +140,13 @@ export class ApolloClient<TCacheShape> implements DataProxy {
cache,
ssrMode = false,
ssrForceFetchDelay = 0,
connectToDevTools,
connectToDevTools =
// Expose the client instance as window.__APOLLO_CLIENT__ and call
// onBroadcast in queryManager.broadcastQueries to enable browser
// devtools, but disable them by default in production.
typeof window === 'object' &&
!(window as any).__APOLLO_CLIENT__ &&
process.env.NODE_ENV !== 'production',
queryDeduplication = true,
defaultOptions,
assumeImmutableResults = false,
Expand Down Expand Up @@ -187,18 +193,7 @@ export class ApolloClient<TCacheShape> implements DataProxy {
this.resetStore = this.resetStore.bind(this);
this.reFetchObservableQueries = this.reFetchObservableQueries.bind(this);

// Attach the client instance to window to let us be found by chrome devtools, but only in
// development mode
const defaultConnectToDevTools =
process.env.NODE_ENV !== 'production' &&
typeof window !== 'undefined' &&
!(window as any).__APOLLO_CLIENT__;

if (
typeof connectToDevTools === 'undefined'
? defaultConnectToDevTools
: connectToDevTools && typeof window !== 'undefined'
) {
if (connectToDevTools) {
(window as any).__APOLLO_CLIENT__ = this;
}

Expand Down Expand Up @@ -253,18 +248,18 @@ export class ApolloClient<TCacheShape> implements DataProxy {
},
localState: this.localState,
assumeImmutableResults,
onBroadcast: () => {
onBroadcast: connectToDevTools ? () => {
if (this.devToolsHookCb) {
this.devToolsHookCb({
action: {},
state: {
queries: this.queryManager.getQueryStore(),
mutations: this.queryManager.mutationStore.getStore(),
mutations: this.queryManager.mutationStore || {},
Comment on lines +251 to +257
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!

},
dataWithOptimisticResults: this.cache.extract(true),
});
}
},
} : void 0,
});
}

Expand Down
53 changes: 0 additions & 53 deletions src/core/MutationStore.ts
View file
Open in desktop

This file was deleted.

54 changes: 38 additions & 16 deletions src/core/QueryManager.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,6 @@ import {
ConcastSourcesIterable,
} from '../utilities';
import { ApolloError, isApolloError } from '../errors';
import { MutationStore } from './MutationStore';
import {
QueryOptions,
WatchQueryOptions,
Expand All @@ -42,18 +41,27 @@ import { QueryInfo, QueryStoreValue, shouldWriteResult } from './QueryInfo';

const { hasOwnProperty } = Object.prototype;

interface MutationStoreValue {
mutation: DocumentNode;
variables: Record<string, any>;
loading: boolean;
error: Error | null;
}

export class QueryManager<TStore> {
public cache: ApolloCache<TStore>;
public link: ApolloLink;
public mutationStore: MutationStore = new MutationStore();
public readonly assumeImmutableResults: boolean;
public readonly ssrMode: boolean;

private queryDeduplication: boolean;
private clientAwareness: Record<string, string> = {};
private localState: LocalState<TStore>;

private onBroadcast: () => void;
private onBroadcast?: () => void;
public mutationStore?: {
[mutationId: string]: MutationStoreValue;
};

// All the queries that the QueryManager is currently managing (not
// including mutations and subscriptions).
Expand All @@ -67,7 +75,7 @@ export class QueryManager<TStore> {
cache,
link,
queryDeduplication = false,
onBroadcast = () => undefined,
onBroadcast,
ssrMode = false,
clientAwareness = {},
localState,
Expand All @@ -85,11 +93,13 @@ export class QueryManager<TStore> {
this.cache = cache;
this.link = link;
this.queryDeduplication = queryDeduplication;
this.onBroadcast = onBroadcast;
this.clientAwareness = clientAwareness;
this.localState = localState || new LocalState({ cache });
this.ssrMode = ssrMode;
this.assumeImmutableResults = !!assumeImmutableResults;
if ((this.onBroadcast = onBroadcast)) {
this.mutationStore = Object.create(null);
}
}

/**
Expand Down Expand Up @@ -142,11 +152,14 @@ export class QueryManager<TStore> {
variables = await this.localState.addExportedVariables(mutation, variables, context);
}

this.mutationStore.initMutation(
mutationId,
mutation,
variables,
);
const mutationStoreValue =
this.mutationStore &&
(this.mutationStore[mutationId] = {
mutation,
variables,
loading: true,
error: null,
} as MutationStoreValue);

if (optimisticResponse) {
this.markMutationOptimistic<T>(optimisticResponse, {
Expand Down Expand Up @@ -184,7 +197,10 @@ export class QueryManager<TStore> {
return;
}

self.mutationStore.markMutationResult(mutationId);
if (mutationStoreValue) {
mutationStoreValue.loading = false;
mutationStoreValue.error = null;
}

if (fetchPolicy !== 'no-cache') {
try {
Expand All @@ -209,7 +225,10 @@ export class QueryManager<TStore> {
},

error(err: Error) {
self.mutationStore.markMutationError(mutationId, err);
if (mutationStoreValue) {
mutationStoreValue.loading = false;
mutationStoreValue.error = err;
}
if (optimisticResponse) {
self.cache.removeOptimistic(mutationId);
}
Expand All @@ -222,8 +241,9 @@ export class QueryManager<TStore> {
},

complete() {
if (error) {
self.mutationStore.markMutationError(mutationId, error);
if (error && mutationStoreValue) {
mutationStoreValue.loading = false;
mutationStoreValue.error = error;
}

if (optimisticResponse) {
Expand Down Expand Up @@ -602,7 +622,9 @@ export class QueryManager<TStore> {
}
});

this.mutationStore.reset();
if (this.mutationStore) {
this.mutationStore = Object.create(null);
}

// begin removing data from the store
return this.cache.reset();
Expand Down Expand Up @@ -733,7 +755,7 @@ export class QueryManager<TStore> {
}

public broadcastQueries() {
this.onBroadcast();
if (this.onBroadcast) this.onBroadcast();
this.queries.forEach(info => info.notify());
}

Expand Down
4 changes: 3 additions & 1 deletion src/core/__tests__/QueryManager/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -74,6 +74,8 @@ describe('QueryManager', () => {
cache: new InMemoryCache({ addTypename: false, ...config }),
clientAwareness,
queryDeduplication,
// Enable client.queryManager.mutationStore tracking.
onBroadcast() {},
});
};

Expand Down Expand Up @@ -3265,7 +3267,7 @@ describe('QueryManager', () => {
queryManager.cache.extract(),
).toEqual({});
expect(queryManager.getQueryStore()).toEqual({});
expect(queryManager.mutationStore.getStore()).toEqual({});
expect(queryManager.mutationStore).toEqual({});

resolve();
});
Expand Down