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

Stop delivering "stale" results to ObservableQuery. #6058

Merged
merged 9 commits into from
Apr 6, 2020
3 changes: 3 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,9 @@
- **[BREAKING]** `client|cache.writeData` have been fully removed. `writeData` usage is one of the easiest ways to turn faulty assumptions about how the cache represents data internally, into cache inconsistency and corruption. `client|cache.writeQuery`, `client|cache.writeFragment`, and/or `cache.modify` can be used to update the cache. <br/>
[@benjamn](https://github.com/benjamn) in [#5923](https://github.com/apollographql/apollo-client/pull/5923)

- **[BREAKING]** Apollo Client will no longer deliver "stale" results to `ObservableQuery` consumers, but will instead log more helpful errors about which cache fields were missing. <br/>
[@benjamn](https://github.com/benjamn) in [#6058](https://github.com/apollographql/apollo-client/pull/6058)

- `InMemoryCache` now supports tracing garbage collection and eviction. Note that the signature of the `evict` method has been simplified in a potentially backwards-incompatible way. <br/>
[@benjamn](https://github.com/benjamn) in [#5310](https://github.com/apollographql/apollo-client/pull/5310)

Expand Down
26 changes: 9 additions & 17 deletions src/__tests__/client.ts
Original file line number Diff line number Diff line change
Expand Up @@ -1654,14 +1654,10 @@ describe('client', () => {
});

subscribeAndCount(reject, obs, (handleCount, result) => {
if (handleCount === 1) {
expect(result.data).toBe(undefined);
expect(result.loading).toBe(true);
} else if (handleCount === 2) {
expect(stripSymbols(result.data)).toEqual(networkFetch);
expect(result.loading).toBe(false);
resolve();
}
expect(handleCount).toBe(1);
expect(stripSymbols(result.data)).toEqual(networkFetch);
expect(result.loading).toBe(false);
resolve();
});
});

Expand All @@ -1677,17 +1673,13 @@ describe('client', () => {
fetchPolicy: 'cache-and-network',
});

let count = 0;
obs.subscribe({
next: result => {
expect(result.data).toBe(undefined);
expect(result.loading).toBe(true);
count++;
},
error: e => {
expect(e.message).toMatch(/No more mocked responses/);
expect(count).toBe(1); // make sure next was called.
setTimeout(resolve, 100);
if (!/No more mocked responses/.test(e.message)) {
reject(e);
} else {
resolve();
}
},
});
});
Expand Down
1 change: 0 additions & 1 deletion src/__tests__/local-state/general.ts
Original file line number Diff line number Diff line change
Expand Up @@ -422,7 +422,6 @@ describe('Cache manipulation', () => {
},
loading: false,
networkStatus: 7,
stale: false,
});

if (selectedItemId !== 123) {
Expand Down
5 changes: 0 additions & 5 deletions src/__tests__/subscribeToMore.ts
Original file line number Diff line number Diff line change
Expand Up @@ -101,7 +101,6 @@ describe('subscribeToMore', () => {
data: { entry: { value: 'Amanda Liu' } },
loading: false,
networkStatus: 7,
stale: false,
});
resolve();
}, 15);
Expand Down Expand Up @@ -157,7 +156,6 @@ describe('subscribeToMore', () => {
data: { entry: { value: 'Amanda Liu' } },
loading: false,
networkStatus: 7,
stale: false,
});
expect(counter).toBe(2);
expect(errorCount).toBe(1);
Expand Down Expand Up @@ -217,7 +215,6 @@ describe('subscribeToMore', () => {
data: { entry: { value: '1' } },
loading: false,
networkStatus: 7,
stale: false,
});
expect(counter).toBe(1);
expect(errorCount).toBe(1);
Expand Down Expand Up @@ -323,7 +320,6 @@ describe('subscribeToMore', () => {
},
loading: false,
networkStatus: 7,
stale: false,
});
resolve();
});
Expand Down Expand Up @@ -391,7 +387,6 @@ describe('subscribeToMore', () => {
data: { entry: { value: 'Amanda Liu' } },
loading: false,
networkStatus: 7,
stale: false,
});
resolve();
}, 15);
Expand Down
5 changes: 4 additions & 1 deletion src/cache/core/types/DataProxy.ts
Original file line number Diff line number Diff line change
@@ -1,5 +1,7 @@
import { DocumentNode } from 'graphql'; // eslint-disable-line import/no-extraneous-dependencies, import/no-unresolved

import { MissingFieldError } from './common';

export namespace DataProxy {
export interface Query<TVariables> {
/**
Expand Down Expand Up @@ -70,7 +72,8 @@ export namespace DataProxy {
export type DiffResult<T> = {
result?: T;
complete?: boolean;
};
missing?: MissingFieldError[];
}
}

/**
Expand Down
11 changes: 11 additions & 0 deletions src/cache/core/types/common.ts
Original file line number Diff line number Diff line change
@@ -1,3 +1,5 @@
import { DocumentNode } from 'graphql';

import {
isReference,
StoreValue,
Expand Down Expand Up @@ -31,3 +33,12 @@ export type Modifier<T> = (value: T, details: {
export type Modifiers = {
[fieldName: string]: Modifier<any>;
}

export class MissingFieldError {
constructor(
public readonly message: string,
public readonly path: (string | number)[],
public readonly query: DocumentNode,
public readonly variables: Record<string, any>,
) {}
}
1 change: 1 addition & 0 deletions src/cache/index.ts
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
export { Transaction, ApolloCache } from './core/cache';
export { Cache } from './core/types/Cache';
export { DataProxy } from './core/types/DataProxy';
export { MissingFieldError } from './core/types/common';

export {
Reference,
Expand Down
34 changes: 34 additions & 0 deletions src/cache/inmemory/__tests__/entityStore.ts
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ import { Policies } from '../policies';
import { StoreObject } from '../types';
import { ApolloCache } from '../../core/cache';
import { Reference } from '../../../utilities/graphql/storeUtils';
import { MissingFieldError } from '../..';

describe('EntityStore', () => {
it('should support result caching if so configured', () => {
Expand Down Expand Up @@ -1065,6 +1066,20 @@ describe('EntityStore', () => {
result: {
authorOfBook: tedWithoutHobby,
},
missing: [
new MissingFieldError(
'Can\'t find field \'hobby\' on Author:{"name":"Ted Chiang"} object',
["authorOfBook", "hobby"],
expect.anything(),
expect.anything(),
),
new MissingFieldError(
'Can\'t find field \'publisherOfBook\' on ROOT_QUERY object',
["publisherOfBook"],
expect.anything(),
expect.anything(),
),
],
});

cache.evict("ROOT_QUERY", "authorOfBook");
Expand Down Expand Up @@ -1296,12 +1311,22 @@ describe('EntityStore', () => {

expect(cache.evict(authorId)).toBe(false);

const missing = [
new MissingFieldError(
"Dangling reference to missing Author:2 object",
["book", "author"],
expect.anything(),
expect.anything(),
),
];

expect(cache.diff({
query,
optimistic: true,
returnPartialData: true,
})).toEqual({
complete: false,
missing,
result: {
book: {
__typename: "Book",
Expand Down Expand Up @@ -1331,6 +1356,7 @@ describe('EntityStore', () => {
returnPartialData: true,
})).toEqual({
complete: false,
missing,
result: {
book: {
__typename: "Book",
Expand Down Expand Up @@ -1553,6 +1579,14 @@ describe('EntityStore', () => {
isbn: "031648637X",
},
},
missing: [
new MissingFieldError(
'Can\'t find field \'title\' on Book:{"isbn":"031648637X"} object',
["book", "title"],
expect.anything(),
expect.anything(),
),
],
});

expect(cache.extract()).toEqual({
Expand Down
23 changes: 23 additions & 0 deletions src/cache/inmemory/__tests__/policies.ts
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ import gql from "graphql-tag";
import { InMemoryCache } from "../inMemoryCache";
import { Policies } from "../policies";
import { Reference, StoreObject } from "../../../core";
import { MissingFieldError } from "../..";

function reverse(s: string) {
return s.split("").reverse().join("");
Expand Down Expand Up @@ -997,6 +998,15 @@ describe("type policies", function () {

expect(cache.extract()).toEqual(snapshot1);

function makeMissingError(jobNumber: number) {
return new MissingFieldError(
`Can't find field 'result' on Job:{"name":"Job #${jobNumber}"} object`,
["jobs", jobNumber - 1, "result"],
expect.anything(),
expect.anything(),
);
}

expect(cache.diff({
query,
optimistic: false,
Expand All @@ -1015,6 +1025,11 @@ describe("type policies", function () {
}],
},
complete: false,
missing: [
makeMissingError(1),
makeMissingError(2),
makeMissingError(3),
],
});

function setResult(jobNum: number) {
Expand Down Expand Up @@ -1061,6 +1076,10 @@ describe("type policies", function () {
}],
},
complete: false,
missing: [
makeMissingError(1),
makeMissingError(3),
],
});

cache.writeQuery({
Expand Down Expand Up @@ -1114,6 +1133,10 @@ describe("type policies", function () {
}],
},
complete: false,
missing: [
makeMissingError(1),
makeMissingError(3),
],
});

setResult(1);
Expand Down
Loading