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

Avoid costly cloneDeep snapshots when cache results are known to be immutable. #4543

Merged
merged 4 commits into from
Mar 7, 2019
Merged

Avoid costly cloneDeep snapshots when cache results are known to be immutable. #4543

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
48ea511
Create QueryManager in ApolloClient constructor.
benjamn Mar 1, 2019
29c0fa1
Avoid costly cloneDeep calls if { assumeImmutableResults: true }.
benjamn Mar 1, 2019
b0a9e4c
Update apollo-boost test that relied on private clientAwareness prope…
benjamn Mar 6, 2019
1e740be
Use invariant.warn to forecast initQueryManager deprecation.
benjamn Mar 6, 2019
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
5 changes: 3 additions & 2 deletions packages/apollo-boost/src/__tests__/config.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -145,8 +145,9 @@ describe('config', () => {
version,
});

expect(client.clientAwareness.name).toEqual(name);
expect(client.clientAwareness.version).toEqual(version);
const { clientAwareness } = client.queryManager as any;
expect(clientAwareness.name).toEqual(name);
expect(clientAwareness.version).toEqual(version);
});

const makePromise = res =>
Expand Down
118 changes: 52 additions & 66 deletions packages/apollo-client/src/ApolloClient.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -55,6 +55,7 @@ export type ApolloClientOptions<TCacheShape> = {
connectToDevTools?: boolean;
queryDeduplication?: boolean;
defaultOptions?: DefaultOptions;
assumeImmutableResults?: boolean;
resolvers?: Resolvers | Resolvers[];
typeDefs?: string | string[] | DocumentNode | DocumentNode[];
fragmentMatcher?: FragmentMatcher;
Expand All @@ -72,19 +73,16 @@ export default class ApolloClient<TCacheShape> implements DataProxy {
public link: ApolloLink;
public store: DataStore<TCacheShape>;
public cache: ApolloCache<TCacheShape>;
public queryManager: QueryManager<TCacheShape> | undefined;
public readonly queryManager: QueryManager<TCacheShape>;
Copy link
Member

Choose a reason for hiding this comment

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

I'm really happy to see this, but do you think we need to worry about this potentially impacting backcompat? People really shouldn't be setting their own queryManager, but we've unfortunately allowed it for a while.

Copy link
Member Author

Choose a reason for hiding this comment

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

@danilobuerger Any thoughts on how risky adding readonly might be?

Copy link
Contributor

Choose a reason for hiding this comment

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

What @hwillson said, they shouldn't be setting it, but they might have. But since its just a typing change, its still settable if someone really wanted to. So I don't see an issue with that.

Copy link
Member Author

Choose a reason for hiding this comment

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

After sleeping on it (and doing a code search for initQueryManager and queryManager), here's my assessment of the use cases:

  • By far the most common use case for calling client.initQueryManager() is simply to ensure client.queryManager exists so it can be examined before the first query happens (e.g. to save a reference to client.queryManager.queryStore). These changes render that effort unnecessary.
  • Mistakenly calling initQueryManager in an attempt to reset client state. This still works as well as it ever did, because initQueryManager was previously idempotent, and now does nothing.
  • Hypothetical use cases that I haven't actually found in the wild:
    • Setting client.queryManager = null and later calling client.initQueryManager() to recreate it. This no longer works because initQueryManager won't recreate the QueryManager now, so adding readonly to client.queryManager seems like a helpful signal to reconsider this pattern.
    • Saving client.queryManager somewhere, setting it to null temporarily, and then restoring it to the old value later. This still works, as long as you circumvent the readonly restriction with client as any, which seems like a reasonable "I know what I'm doing" requirement.

Copy link
Contributor

Choose a reason for hiding this comment

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

Good assessment, the only other thing I could come up with is mocking it away for some reason in tests. (However, I don't know why one would want to do that)

public disableNetworkFetches: boolean;
public version: string;
public queryDeduplication: boolean;
public defaultOptions: DefaultOptions = {};
public readonly typeDefs: ApolloClientOptions<TCacheShape>['typeDefs'];

private devToolsHookCb: Function;
private proxy: ApolloCache<TCacheShape> | undefined;
private ssrMode: boolean;
private resetStoreCallbacks: Array<() => Promise<any>> = [];
private clearStoreCallbacks: Array<() => Promise<any>> = [];
private clientAwareness: Record<string, string> = {};
private localState: LocalState<TCacheShape>;

/**
Expand All @@ -106,6 +104,12 @@ export default class ApolloClient<TCacheShape> implements DataProxy {
* options supplied to `watchQuery`, `query`, or
* `mutate`.
*
* @param assumeImmutableResults When this option is true, the client will assume results
* read from the cache are never mutated by application code,
* which enables substantial performance optimizations. Passing
* `{ freezeResults: true }` to the `InMemoryCache` constructor
* can help enforce this immutability.
*
* @param name A custom name that can be used to identify this client, when
* using Apollo client awareness features. E.g. "iOS".
*
Expand All @@ -123,6 +127,7 @@ export default class ApolloClient<TCacheShape> implements DataProxy {
connectToDevTools,
queryDeduplication = true,
defaultOptions,
assumeImmutableResults = false,
resolvers,
typeDefs,
fragmentMatcher,
Expand Down Expand Up @@ -166,7 +171,6 @@ export default class ApolloClient<TCacheShape> implements DataProxy {
this.store = new DataStore(cache);
this.disableNetworkFetches = ssrMode || ssrForceFetchDelay > 0;
this.queryDeduplication = queryDeduplication;
this.ssrMode = ssrMode;
this.defaultOptions = defaultOptions || {};
this.typeDefs = typeDefs;

Expand Down Expand Up @@ -231,20 +235,37 @@ export default class ApolloClient<TCacheShape> implements DataProxy {

this.version = version;

if (clientAwarenessName) {
this.clientAwareness.name = clientAwarenessName;
}

if (clientAwarenessVersion) {
this.clientAwareness.version = clientAwarenessVersion;
}

this.localState = new LocalState({
cache,
client: this,
resolvers,
fragmentMatcher,
});

this.queryManager = new QueryManager({
link: this.link,
store: this.store,
queryDeduplication,
ssrMode,
clientAwareness: {
name: clientAwarenessName!,
version: clientAwarenessVersion!,
},
localState: this.localState,
assumeImmutableResults,
onBroadcast: () => {
if (this.devToolsHookCb) {
this.devToolsHookCb({
action: {},
state: {
queries: this.queryManager.queryStore.getStore(),
mutations: this.queryManager.mutationStore.getStore(),
},
dataWithOptimisticResults: this.cache.extract(true),
});
}
},
});
}

/**
Expand Down Expand Up @@ -295,7 +316,7 @@ export default class ApolloClient<TCacheShape> implements DataProxy {
options = { ...options, fetchPolicy: 'cache-first' };
}

return this.initQueryManager().watchQuery<T, TVariables>(options);
return this.queryManager.watchQuery<T, TVariables>(options);
}

/**
Expand Down Expand Up @@ -327,7 +348,7 @@ export default class ApolloClient<TCacheShape> implements DataProxy {
options = { ...options, fetchPolicy: 'cache-first' };
}

return this.initQueryManager().query<T>(options);
return this.queryManager.query<T>(options);
}

/**
Expand All @@ -347,7 +368,7 @@ export default class ApolloClient<TCacheShape> implements DataProxy {
} as MutationOptions<T, TVariables>;
}

return this.initQueryManager().mutate<T>(options);
return this.queryManager.mutate<T>(options);
}

/**
Expand All @@ -357,7 +378,7 @@ export default class ApolloClient<TCacheShape> implements DataProxy {
public subscribe<T = any, TVariables = OperationVariables>(
options: SubscriptionOptions<TVariables>,
): Observable<T> {
return this.initQueryManager().startGraphQLSubscription<T>(options);
return this.queryManager.startGraphQLSubscription<T>(options);
}

/**
Expand All @@ -373,7 +394,7 @@ export default class ApolloClient<TCacheShape> implements DataProxy {
options: DataProxy.Query<TVariables>,
optimistic: boolean = false,
): T | null {
return this.initProxy().readQuery<T, TVariables>(options, optimistic);
return this.cache.readQuery<T, TVariables>(options, optimistic);
}

/**
Expand All @@ -394,7 +415,7 @@ export default class ApolloClient<TCacheShape> implements DataProxy {
options: DataProxy.Fragment<TVariables>,
optimistic: boolean = false,
): T | null {
return this.initProxy().readFragment<T, TVariables>(options, optimistic);
return this.cache.readFragment<T, TVariables>(options, optimistic);
}

/**
Expand All @@ -405,8 +426,8 @@ export default class ApolloClient<TCacheShape> implements DataProxy {
public writeQuery<TData = any, TVariables = OperationVariables>(
options: DataProxy.WriteQueryOptions<TData, TVariables>,
): void {
const result = this.initProxy().writeQuery<TData, TVariables>(options);
this.initQueryManager().broadcastQueries();
const result = this.cache.writeQuery<TData, TVariables>(options);
this.queryManager.broadcastQueries();
return result;
}

Expand All @@ -424,8 +445,8 @@ export default class ApolloClient<TCacheShape> implements DataProxy {
public writeFragment<TData = any, TVariables = OperationVariables>(
options: DataProxy.WriteFragmentOptions<TData, TVariables>,
): void {
const result = this.initProxy().writeFragment<TData, TVariables>(options);
this.initQueryManager().broadcastQueries();
const result = this.cache.writeFragment<TData, TVariables>(options);
this.queryManager.broadcastQueries();
return result;
}

Expand All @@ -442,8 +463,8 @@ export default class ApolloClient<TCacheShape> implements DataProxy {
public writeData<TData = any>(
options: DataProxy.WriteDataOptions<TData>,
): void {
const result = this.initProxy().writeData<TData>(options);
this.initQueryManager().broadcastQueries();
const result = this.cache.writeData<TData>(options);
this.queryManager.broadcastQueries();
return result;
}

Expand All @@ -459,32 +480,10 @@ export default class ApolloClient<TCacheShape> implements DataProxy {
* This initializes the query manager that tracks queries and the cache
*/
public initQueryManager(): QueryManager<TCacheShape> {
benjamn marked this conversation as resolved.
Show resolved Hide resolved
if (!this.queryManager) {
this.queryManager = new QueryManager({
link: this.link,
store: this.store,
queryDeduplication: this.queryDeduplication,
ssrMode: this.ssrMode,
clientAwareness: this.clientAwareness,
localState: this.localState,
onBroadcast: () => {
if (this.devToolsHookCb) {
this.devToolsHookCb({
action: {},
state: {
queries: this.queryManager
? this.queryManager.queryStore.getStore()
: {},
mutations: this.queryManager
? this.queryManager.mutationStore.getStore()
: {},
},
dataWithOptimisticResults: this.cache.extract(true),
});
}
},
});
}
invariant.warn(
'Calling the initQueryManager method is no longer necessary, ' +
'and it will be removed from ApolloClient in version 3.0.',
);
return this.queryManager;
}

Expand Down Expand Up @@ -581,7 +580,7 @@ export default class ApolloClient<TCacheShape> implements DataProxy {
* Exposes the cache's complete state, in a serializable format for later restoration.
*/
public extract(optimistic?: boolean): TCacheShape {
return this.initProxy().extract(optimistic);
return this.cache.extract(optimistic);
}

/**
Expand All @@ -592,7 +591,7 @@ export default class ApolloClient<TCacheShape> implements DataProxy {
* and also (potentially) during hot reloads.
*/
public restore(serializedState: TCacheShape): ApolloCache<TCacheShape> {
return this.initProxy().restore(serializedState);
return this.cache.restore(serializedState);
}

/**
Expand Down Expand Up @@ -622,17 +621,4 @@ export default class ApolloClient<TCacheShape> implements DataProxy {
public setLocalStateFragmentMatcher(fragmentMatcher: FragmentMatcher) {
this.localState.setFragmentMatcher(fragmentMatcher);
}

/**
* Initializes a data proxy for this client instance if one does not already
* exist and returns either a previously initialized proxy instance or the
* newly initialized instance.
*/
private initProxy(): ApolloCache<TCacheShape> {
if (!this.proxy) {
this.initQueryManager();
this.proxy = this.cache;
}
return this.proxy;
}
}
18 changes: 0 additions & 18 deletions packages/apollo-client/src/__tests__/client.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,20 +22,6 @@ import { withWarning } from '../util/wrap';
import { mockSingleLink } from '../__mocks__/mockLinks';

describe('client', () => {
it('creates query manager lazily', () => {
const client = new ApolloClient({
link: ApolloLink.empty(),
cache: new InMemoryCache(),
});

expect(client.queryManager).toBeUndefined();

// We only create the query manager on the first query
client.initQueryManager();
expect(client.queryManager).toBeDefined();
expect(client.cache).toBeDefined();
});

it('can be loaded via require', () => {
/* tslint:disable */
const ApolloClientRequire = require('../').default;
Expand All @@ -46,10 +32,6 @@ describe('client', () => {
cache: new InMemoryCache(),
});

expect(client.queryManager).toBeUndefined();

// We only create the query manager on the first query
client.initQueryManager();
expect(client.queryManager).toBeDefined();
expect(client.cache).toBeDefined();
});
Expand Down
6 changes: 4 additions & 2 deletions packages/apollo-client/src/core/ObservableQuery.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -250,7 +250,8 @@ export class ObservableQuery<

if (!partial) {
this.lastResult = { ...result, stale: false };
this.lastResultSnapshot = cloneDeep(this.lastResult);
this.lastResultSnapshot = this.queryManager.assumeImmutableResults
? this.lastResult : cloneDeep(this.lastResult);
}

return { ...result, partial };
Expand Down Expand Up @@ -611,7 +612,8 @@ export class ObservableQuery<
const observer: Observer<ApolloQueryResult<TData>> = {
next: (result: ApolloQueryResult<TData>) => {
this.lastResult = result;
this.lastResultSnapshot = cloneDeep(result);
this.lastResultSnapshot = this.queryManager.assumeImmutableResults
? result : cloneDeep(result);
this.observers.forEach(obs => obs.next && obs.next(result));
},
error: (error: ApolloError) => {
Expand Down
4 changes: 4 additions & 0 deletions packages/apollo-client/src/core/QueryManager.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -57,6 +57,7 @@ export class QueryManager<TStore> {
public mutationStore: MutationStore = new MutationStore();
public queryStore: QueryStore = new QueryStore();
public dataStore: DataStore<TStore>;
public readonly assumeImmutableResults: boolean;

private deduplicator: ApolloLink;
private queryDeduplication: boolean;
Expand Down Expand Up @@ -94,6 +95,7 @@ export class QueryManager<TStore> {
ssrMode = false,
clientAwareness = {},
localState,
assumeImmutableResults,
}: {
link: ApolloLink;
queryDeduplication?: boolean;
Expand All @@ -102,6 +104,7 @@ export class QueryManager<TStore> {
ssrMode?: boolean;
clientAwareness?: Record<string, string>;
localState?: LocalState<TStore>;
assumeImmutableResults?: boolean;
}) {
this.link = link;
this.deduplicator = ApolloLink.from([new Deduplicator(), link]);
Expand All @@ -111,6 +114,7 @@ export class QueryManager<TStore> {
this.clientAwareness = clientAwareness;
this.localState = localState || new LocalState({ cache: store.getCache() });
this.ssrMode = ssrMode;
this.assumeImmutableResults = !!assumeImmutableResults;
}

/**
Expand Down
Loading