Skip to content

Commit

Permalink
Completely remove unnecessary ID_KEY Symbol.
Browse files Browse the repository at this point in the history 
Restoring non-enumerability of the ID_KEY Symbol in #3544 made ID_KEY
slightly more hidden from application code, at the cost of slightly worse
performance (because of Object.defineProperty), but tests were still
broken because Jest now includes Symbol keys when checking object equality
(even the non-enumerable ones).

Fortunately, given all the previousResult refactoring that has happened in
PR #3394, we no longer need to store ID_KEY properties at all, which
completely side-steps the question of whether ID_KEY should be enumerable
or not, and avoids any problems due to Jest including Symbol keys when
checking deep equality.

If we decide to bring this ID metadata back in the future, we could use a
WeakMap to associate result objects with their IDs, so that we can avoid
modifying the result objects.
  • Loading branch information
@benjamn
benjamn committed Jun 6, 2018
1 parent 7314c1e commit 8b2ab9b
Show file tree
Hide file tree
Showing 2 changed files with 31 additions and 48 deletions.
62 changes: 31 additions & 31 deletions packages/apollo-cache-inmemory/src/__tests__/diffAgainstStore.ts
View file
Original file line number Diff line number Diff line change
@@ -1,9 +1,8 @@
import gql, { disableFragmentWarnings } from 'graphql-tag';
import { toIdValue, stripSymbols } from 'apollo-utilities';
import { toIdValue } from 'apollo-utilities';

import { defaultNormalizedCacheFactory } from '../objectCache';
import { diffQueryAgainstStore } from '../readFromStore';
import { ID_KEY } from '../executeStoreQuery';
import { writeQueryToStore } from '../writeToStore';
import { HeuristicFragmentMatcher } from '../fragmentMatcher';
import { defaultDataIdFromObject } from '../inMemoryCache';
Expand Down Expand Up @@ -415,7 +414,7 @@ describe('diffing queries against the store', () => {
query: simpleQuery,
});

expect(stripSymbols(simpleDiff.result)).toEqual({
expect(simpleDiff.result).toEqual({
people_one: {
name: 'Luke Skywalker',
},
Expand All @@ -426,7 +425,7 @@ describe('diffing queries against the store', () => {
query: inlineFragmentQuery,
});

expect(stripSymbols(inlineDiff.result)).toEqual({
expect(inlineDiff.result).toEqual({
people_one: {
name: 'Luke Skywalker',
},
Expand All @@ -437,7 +436,7 @@ describe('diffing queries against the store', () => {
query: namedFragmentQuery,
});

expect(stripSymbols(namedDiff.result)).toEqual({
expect(namedDiff.result).toEqual({
people_one: {
name: 'Luke Skywalker',
},
Expand Down Expand Up @@ -487,29 +486,30 @@ describe('diffing queries against the store', () => {
},
};

function dataIdFromObject({ id }: { id: string}) {
return id;
}

const store = writeQueryToStore({
query,
result: queryResult,
dataIdFromObject: ({ id }: { id: string }) => id,
dataIdFromObject,
});

const { result } = diffQueryAgainstStore({
store,
query,
});

expect(stripSymbols(result)).toEqual(queryResult);
expect(result[ID_KEY]).toBe('ROOT_QUERY');
expect(result.a[0][ID_KEY]).toBe('a:1');
expect(result.a[1][ID_KEY]).toBe('a:2');
expect(result.a[2][ID_KEY]).toBe('a:3');
expect(result.c[ID_KEY]).toBe('$ROOT_QUERY.c');
expect(result.c.e[0][ID_KEY]).toBe('e:1');
expect(result.c.e[1][ID_KEY]).toBe('e:2');
expect(result.c.e[2][ID_KEY]).toBe('e:3');
expect(result.c.e[3][ID_KEY]).toBe('e:4');
expect(result.c.e[4][ID_KEY]).toBe('e:5');
expect(result.c.g[ID_KEY]).toBe('$ROOT_QUERY.c.g');
expect(result).toEqual(queryResult);
expect(dataIdFromObject(result.a[0])).toBe('a:1');
expect(dataIdFromObject(result.a[1])).toBe('a:2');
expect(dataIdFromObject(result.a[2])).toBe('a:3');
expect(dataIdFromObject(result.c.e[0])).toBe('e:1');
expect(dataIdFromObject(result.c.e[1])).toBe('e:2');
expect(dataIdFromObject(result.c.e[2])).toBe('e:3');
expect(dataIdFromObject(result.c.e[3])).toBe('e:4');
expect(dataIdFromObject(result.c.e[4])).toBe('e:5');
});

describe('referential equality preservation', () => {
Expand Down Expand Up @@ -589,7 +589,7 @@ describe('diffing queries against the store', () => {
previousResult,
});

expect(stripSymbols(result)).toEqual(queryResult);
expect(result).toEqual(queryResult);
expect(result).not.toEqual(previousResult);
expect(result.a).toEqual(previousResult.a);
expect(result.c).not.toEqual(previousResult.c);
Expand Down Expand Up @@ -670,7 +670,7 @@ describe('diffing queries against the store', () => {
previousResult,
});

expect(stripSymbols(result)).toEqual(queryResult);
expect(result).toEqual(queryResult);
expect(result.a[0]).toEqual(previousResult.a[0]);
expect(result.a[1]).toEqual(previousResult.a[1]);
});
Expand Down Expand Up @@ -763,7 +763,7 @@ describe('diffing queries against the store', () => {
previousResult,
});

expect(stripSymbols(result)).toEqual(queryResult);
expect(result).toEqual(queryResult);
expect(result).not.toEqual(previousResult);
expect(result.a).not.toEqual(previousResult.a);
expect(result.a[0]).toEqual(previousResult.a[0]);
Expand Down Expand Up @@ -825,18 +825,18 @@ describe('diffing queries against the store', () => {

const previousResult = {
a: [
{ id: 'a:3', b: 1.3, [ID_KEY]: 'a:3' },
{ id: 'a:2', b: 1.2, [ID_KEY]: 'a:2' },
{ id: 'a:1', b: 1.1, [ID_KEY]: 'a:1' },
{ id: 'a:3', b: 1.3 },
{ id: 'a:2', b: 1.2 },
{ id: 'a:1', b: 1.1 },
],
c: {
d: 2,
e: [
{ id: 'e:4', f: 3.4, [ID_KEY]: 'e:4' },
{ id: 'e:2', f: 3.2, [ID_KEY]: 'e:2' },
{ id: 'e:5', f: 3.5, [ID_KEY]: 'e:5' },
{ id: 'e:3', f: 3.3, [ID_KEY]: 'e:3' },
{ id: 'e:1', f: 3.1, [ID_KEY]: 'e:1' },
{ id: 'e:4', f: 3.4 },
{ id: 'e:2', f: 3.2 },
{ id: 'e:5', f: 3.5 },
{ id: 'e:3', f: 3.3 },
{ id: 'e:1', f: 3.1 },
],
g: { h: 4 },
},
Expand All @@ -848,7 +848,7 @@ describe('diffing queries against the store', () => {
previousResult,
});

expect(stripSymbols(result)).toEqual(queryResult);
expect(result).toEqual(queryResult);
expect(result).not.toEqual(previousResult);
expect(result.a).not.toEqual(previousResult.a);
expect(result.a[0]).toEqual(previousResult.a[2]);
Expand Down Expand Up @@ -899,7 +899,7 @@ describe('diffing queries against the store', () => {
previousResult,
});

expect(stripSymbols(result)).toEqual(queryResult);
expect(result).toEqual(queryResult);
expect(result).not.toEqual(previousResult);
expect(result.a).toEqual(previousResult.a);
expect(result.d).not.toEqual(previousResult.d);
Expand Down
17 changes: 0 additions & 17 deletions packages/apollo-cache-inmemory/src/executeStoreQuery.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -34,16 +34,6 @@ import {
import { wrap, defaultMakeCacheKey } from './optimism';
import { DepTrackingCache } from './depTrackingCache';

/**
* The key which the cache id for a given value is stored in the result object. This key is private
* and should not be used by Apollo client users.
*
* Uses a symbol if available in the environment.
*
* @private
*/
export const ID_KEY = typeof Symbol !== 'undefined' ? Symbol('id') : '@@id';

export type VariableMap = { [name: string]: any };

export type FragmentMatcher = (
Expand Down Expand Up @@ -291,13 +281,6 @@ const executeSelectionSet = wrap(function _executeSelectionSet(
}
});

Object.defineProperty(finalResult.result, ID_KEY, {
enumerable: false,
configurable: true,
writable: false,
value: rootValue.id,
});

return finalResult;

}, {
Expand Down

0 comments on commit 8b2ab9b

Please sign in to comment.