Skip to content

Commit

Permalink
Eliminate cache.writeData, a hack worse than the alternatives.
Browse files Browse the repository at this point in the history 
I referred to cache.writeData as a "foot-seeking missile" in PR #5909,
because it's one of the easiest ways to turn your faulty assumptions about
how the cache represents data internally into cache corruption.

PR #5909 introduced an alternative api, cache.modify(id, modifiers), which
aims to take the place of the more "surgical" uses of cache.writeData.
However, as you can see, in almost every case where cache.writeData was
used in our tests, an appropriate query was already sitting very close by,
making cache.writeQuery just as easy to call.

If you think your life is worse now that you have to pass a query to
cache.writeQuery or a fragment to cache.writeFragment, please realize that
cache.writeData was dynamically creating a fresh query or fragment behind
the scenes, every time it was called, so it was actually doing a lot more
work than the equivalent call to cache.writeQuery or cache.writeFragment.
  • Loading branch information
@benjamn
benjamn committed Feb 10, 2020
1 parent 549262d commit 6b2df25
Show file tree
Hide file tree
Showing 13 changed files with 83 additions and 655 deletions.
18 changes: 0 additions & 18 deletions src/ApolloClient.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -430,24 +430,6 @@ export class ApolloClient<TCacheShape> implements DataProxy {
return result;
}

/**
* Sugar for writeQuery & writeFragment
* This method will construct a query from the data object passed in.
* If no id is supplied, writeData will write the data to the root.
* If an id is supplied, writeData will write a fragment to the object
* specified by the id in the store.
*
* Since you aren't passing in a query to check the shape of the data,
* you must pass in an object that conforms to the shape of valid GraphQL data.
*/
public writeData<TData = any>(
options: DataProxy.WriteDataOptions<TData>,
): void {
const result = this.cache.writeData<TData>(options);
this.queryManager.broadcastQueries();
return result;
}

public __actionHookForDevTools(cb: () => any) {
this.devToolsHookCb = cb;
}
Expand Down
165 changes: 0 additions & 165 deletions src/__tests__/ApolloClient.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1420,171 +1420,6 @@ describe('ApolloClient', () => {
});
});

describe('writeData', () => {
it('lets you write to the cache by passing in data', () => {
const query = gql`
{
field
}
`;

const client = new ApolloClient({
cache: new InMemoryCache(),
link: ApolloLink.empty(),
});

client.writeData({ data: { field: 1 } });

return client.query({ query }).then(({ data }) => {
expect(stripSymbols({ ...data })).toEqual({ field: 1 });
});
});

it('lets you write to an existing object in the cache using an ID', () => {
const query = gql`
{
obj {
field
}
}
`;

const client = new ApolloClient({
cache: new InMemoryCache(),
link: ApolloLink.empty(),
});

client.writeQuery({
query,
data: {
obj: { field: 1, id: 'uniqueId', __typename: 'Object' },
},
});

client.writeData({ id: 'Object:uniqueId', data: { field: 2 } });

return client.query({ query }).then(({ data }: any) => {
expect(data.obj.field).toEqual(2);
});
});

it(`doesn't overwrite __typename when writing to the cache with an id`, () => {
const query = gql`
{
obj {
field {
field2
}
id
}
}
`;

const client = new ApolloClient({
cache: new InMemoryCache(),
link: ApolloLink.empty(),
});

client.writeQuery({
query,
data: {
obj: {
field: { field2: 1, __typename: 'Field' },
id: 'uniqueId',
__typename: 'Object',
},
},
});

client.writeData({
id: 'Object:uniqueId',
data: { field: { field2: 2, __typename: 'Field' } },
});

return client
.query({ query })
.then(({ data }: any) => {
expect(data.obj.__typename).toEqual('Object');
expect(data.obj.field.__typename).toEqual('Field');
})
.catch(e => console.log(e));
});

it(`adds a __typename for an object without one when writing to the cache with an id`, () => {
const query = gql`
{
obj {
field {
field2
}
id
}
}
`;

// This would cause a warning to be printed because we don't have
// __typename on the obj field. But that's intentional because
// that's exactly the situation we're trying to test...

// Let's swap out console.warn to suppress this one message

const suppressString = '__typename';
const originalWarn = console.warn;
console.warn = (...args: any[]) => {
if (
args.find(element => {
if (typeof element === 'string') {
return element.indexOf(suppressString) !== -1;
}
return false;
}) != null
) {
// Found a thing in the args we told it to exclude
return;
}
originalWarn.apply(console, args);
};

const client = new ApolloClient({
cache: new InMemoryCache(),
link: ApolloLink.empty(),
});

client.writeQuery({
query,
data: {
obj: {
__typename: 'Obj',
field: {
field2: 1,
__typename: 'Field',
},
id: 'uniqueId',
},
},
});

client.writeData({
id: 'Obj:uniqueId',
data: {
field: {
field2: 2,
__typename: 'Field',
},
},
});

return client
.query({ query })
.then(({ data }: any) => {
console.warn = originalWarn;
expect(data.obj.__typename).toEqual('Obj');
expect(data.obj.field.__typename).toEqual('Field');
})
.catch(e => console.log(e));
});
});

describe('write then read', () => {
it('will write data locally which will then be read back', () => {
const client = new ApolloClient({
Expand Down
48 changes: 35 additions & 13 deletions src/__tests__/local-state/export.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,11 @@ describe('@client @export tests', () => {
link: ApolloLink.empty(),
resolvers: {},
});
cache.writeData({ data: { field: 1 } });

cache.writeQuery({
query,
data: { field: 1 },
});

return client.query({ query }).then(({ data }: any) => {
expect({ ...data }).toMatchObject({ field: 1 });
Expand Down Expand Up @@ -54,7 +58,8 @@ describe('@client @export tests', () => {
resolvers: {},
});

cache.writeData({
cache.writeQuery({
query,
data: {
car: {
engine: {
Expand Down Expand Up @@ -106,7 +111,8 @@ describe('@client @export tests', () => {
},
});

cache.writeData({
cache.writeQuery({
query,
data: {
currentAuthorId: testAuthorId,
},
Expand Down Expand Up @@ -156,7 +162,8 @@ describe('@client @export tests', () => {
},
});

cache.writeData({
cache.writeQuery({
query,
data: {
currentAuthor: testAuthor,
},
Expand Down Expand Up @@ -206,7 +213,8 @@ describe('@client @export tests', () => {
resolvers: {},
});

cache.writeData({
cache.writeQuery({
query,
data: {
currentAuthor: testAuthor,
},
Expand Down Expand Up @@ -268,7 +276,8 @@ describe('@client @export tests', () => {
resolvers: {},
});

cache.writeData({
cache.writeQuery({
query,
data: {
appContainer,
},
Expand Down Expand Up @@ -371,7 +380,8 @@ describe('@client @export tests', () => {
resolvers: {},
});

cache.writeData({
cache.writeQuery({
query,
data: {
postRequiringReview: {
loggedInReviewerId,
Expand Down Expand Up @@ -450,7 +460,8 @@ describe('@client @export tests', () => {
},
});

cache.writeData({
cache.writeQuery({
query,
data: {
postRequiringReview: {
__typename: 'Post',
Expand Down Expand Up @@ -560,7 +571,8 @@ describe('@client @export tests', () => {
resolvers: {},
});

cache.writeData({
cache.writeQuery({
query: gql`{ topPost }`,
data: {
topPost: testPostId,
},
Expand Down Expand Up @@ -685,7 +697,8 @@ describe('@client @export tests', () => {
resolvers: {},
});

cache.writeData({
cache.writeQuery({
query,
data: {
primaryReviewerId,
secondaryReviewerId,
Expand Down Expand Up @@ -736,7 +749,10 @@ describe('@client @export tests', () => {
resolvers: {},
});

client.writeData({ data: { currentAuthorId: testAuthorId1 } });
client.writeQuery({
query,
data: { currentAuthorId: testAuthorId1 },
});

const obs = client.watchQuery({ query });
obs.subscribe({
Expand All @@ -746,7 +762,10 @@ describe('@client @export tests', () => {
currentAuthorId: testAuthorId1,
postCount: testPostCount1,
});
client.writeData({ data: { currentAuthorId: testAuthorId2 } });
client.writeQuery({
query,
data: { currentAuthorId: testAuthorId2 },
});
} else if (resultCount === 1) {
expect({ ...data }).toMatchObject({
currentAuthorId: testAuthorId2,
Expand Down Expand Up @@ -797,7 +816,10 @@ describe('@client @export tests', () => {
resolvers: {},
});

client.writeData({ data: { currentAuthorId: testAuthorId1 } });
client.writeQuery({
query,
data: { currentAuthorId: testAuthorId1 },
});

const obs = client.watchQuery({ query });
obs.subscribe({
Expand Down
16 changes: 11 additions & 5 deletions src/__tests__/local-state/general.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -850,7 +850,8 @@ describe('Combining client and server state/operations', () => {
resolvers: {},
});

cache.writeData({
cache.writeQuery({
query,
data: {
count: 0,
},
Expand Down Expand Up @@ -882,7 +883,7 @@ describe('Combining client and server state/operations', () => {
user: {
__typename: 'User',
// We need an id (or a keyFields policy) because, if the User
// object is not identifiable, the call to cache.writeData
// object is not identifiable, the call to cache.writeQuery
// below will simply replace the existing data rather than
// merging the new data with the existing data.
id: 123,
Expand All @@ -898,7 +899,8 @@ describe('Combining client and server state/operations', () => {
resolvers: {},
});

cache.writeData({
cache.writeQuery({
query,
data: {
user: {
__typename: 'User',
Expand Down Expand Up @@ -984,14 +986,18 @@ describe('Combining client and server state/operations', () => {
incrementCount: (_, __, { cache }) => {
const { count } = cache.readQuery({ query: counterQuery });
const data = { count: count + 1 };
cache.writeData({ data });
cache.writeQuery({
query: counterQuery,
data,
});
return null;
},
},
},
});

cache.writeData({
cache.writeQuery({
query: counterQuery,
data: {
count: 0,
},
Expand Down
Loading

0 comments on commit 6b2df25

Please sign in to comment.