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

Jump to bottom

Remove freezeResults option, assuming always true. #5153

Merged
merged 3 commits into from
Aug 21, 2019
Merged

Remove freezeResults option, assuming always true. #5153

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
6987469
Remove freezeResults option, assuming always true.
benjamn Aug 9, 2019
2643dbb
Fix apollo-client tests after making freezeResults true.
benjamn Aug 9, 2019
af94014
Avoid freezing user-provided scalar field values.
benjamn Aug 21, 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
128 changes: 12 additions & 116 deletions packages/apollo-cache-inmemory/src/__tests__/__snapshots__/cache.ts.snap
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
// Jest Snapshot v1, https://goo.gl/fbAQLP

exports[`Cache writeFragment will write some deeply nested data into the store at any id (1/3) 1`] = `
exports[`Cache writeFragment will write some deeply nested data into the store at any id (1/2) 1`] = `
Object {
"bar": Object {
"i": 7,
Expand All @@ -14,7 +14,7 @@ Object {
}
`;

exports[`Cache writeFragment will write some deeply nested data into the store at any id (1/3) 2`] = `
exports[`Cache writeFragment will write some deeply nested data into the store at any id (1/2) 2`] = `
Object {
"bar": Object {
"i": 7,
Expand All @@ -32,7 +32,7 @@ Object {
}
`;

exports[`Cache writeFragment will write some deeply nested data into the store at any id (1/3) 3`] = `
exports[`Cache writeFragment will write some deeply nested data into the store at any id (1/2) 3`] = `
Object {
"bar": Object {
"i": 10,
Expand All @@ -50,7 +50,7 @@ Object {
}
`;

exports[`Cache writeFragment will write some deeply nested data into the store at any id (1/3) 4`] = `
exports[`Cache writeFragment will write some deeply nested data into the store at any id (1/2) 4`] = `
Object {
"bar": Object {
"i": 10,
Expand All @@ -68,7 +68,7 @@ Object {
}
`;

exports[`Cache writeFragment will write some deeply nested data into the store at any id (1/3) 5`] = `
exports[`Cache writeFragment will write some deeply nested data into the store at any id (1/2) 5`] = `
Object {
"bar": Object {
"i": 7,
Expand All @@ -86,7 +86,7 @@ Object {
}
`;

exports[`Cache writeFragment will write some deeply nested data into the store at any id (1/3) 6`] = `
exports[`Cache writeFragment will write some deeply nested data into the store at any id (1/2) 6`] = `
Object {
"bar": Object {
"i": 10,
Expand All @@ -104,7 +104,7 @@ Object {
}
`;

exports[`Cache writeFragment will write some deeply nested data into the store at any id (2/3) 1`] = `
exports[`Cache writeFragment will write some deeply nested data into the store at any id (2/2) 1`] = `
Object {
"bar": Object {
"i": 7,
Expand All @@ -118,7 +118,7 @@ Object {
}
`;

exports[`Cache writeFragment will write some deeply nested data into the store at any id (2/3) 2`] = `
exports[`Cache writeFragment will write some deeply nested data into the store at any id (2/2) 2`] = `
Object {
"bar": Object {
"i": 7,
Expand All @@ -136,7 +136,7 @@ Object {
}
`;

exports[`Cache writeFragment will write some deeply nested data into the store at any id (2/3) 3`] = `
exports[`Cache writeFragment will write some deeply nested data into the store at any id (2/2) 3`] = `
Object {
"bar": Object {
"i": 10,
Expand All @@ -154,7 +154,7 @@ Object {
}
`;

exports[`Cache writeFragment will write some deeply nested data into the store at any id (2/3) 4`] = `
exports[`Cache writeFragment will write some deeply nested data into the store at any id (2/2) 4`] = `
Object {
"bar": Object {
"i": 10,
Expand All @@ -172,7 +172,7 @@ Object {
}
`;

exports[`Cache writeFragment will write some deeply nested data into the store at any id (2/3) 5`] = `
exports[`Cache writeFragment will write some deeply nested data into the store at any id (2/2) 5`] = `
Object {
"bar": Object {
"i": 7,
Expand All @@ -190,111 +190,7 @@ Object {
}
`;

exports[`Cache writeFragment will write some deeply nested data into the store at any id (2/3) 6`] = `
Object {
"bar": Object {
"i": 10,
"j": 11,
"k": 12,
},
"foo": Object {
"e": 4,
"f": 5,
"g": 6,
"h": Object {
"__ref": "bar",
},
},
}
`;

exports[`Cache writeFragment will write some deeply nested data into the store at any id (3/3) 1`] = `
Object {
"bar": Object {
"i": 7,
},
"foo": Object {
"e": 4,
"h": Object {
"__ref": "bar",
},
},
}
`;

exports[`Cache writeFragment will write some deeply nested data into the store at any id (3/3) 2`] = `
Object {
"bar": Object {
"i": 7,
"j": 8,
"k": 9,
},
"foo": Object {
"e": 4,
"f": 5,
"g": 6,
"h": Object {
"__ref": "bar",
},
},
}
`;

exports[`Cache writeFragment will write some deeply nested data into the store at any id (3/3) 3`] = `
Object {
"bar": Object {
"i": 10,
"j": 8,
"k": 9,
},
"foo": Object {
"e": 4,
"f": 5,
"g": 6,
"h": Object {
"__ref": "bar",
},
},
}
`;

exports[`Cache writeFragment will write some deeply nested data into the store at any id (3/3) 4`] = `
Object {
"bar": Object {
"i": 10,
"j": 11,
"k": 12,
},
"foo": Object {
"e": 4,
"f": 5,
"g": 6,
"h": Object {
"__ref": "bar",
},
},
}
`;

exports[`Cache writeFragment will write some deeply nested data into the store at any id (3/3) 5`] = `
Object {
"bar": Object {
"i": 7,
"j": 8,
"k": 9,
},
"foo": Object {
"e": 4,
"f": 5,
"g": 6,
"h": Object {
"__ref": "bar",
},
},
}
`;

exports[`Cache writeFragment will write some deeply nested data into the store at any id (3/3) 6`] = `
exports[`Cache writeFragment will write some deeply nested data into the store at any id (2/2) 6`] = `
Object {
"bar": Object {
"i": 10,
Expand Down
11 changes: 0 additions & 11 deletions packages/apollo-cache-inmemory/src/__tests__/cache.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,12 +24,6 @@ describe('Cache', () => {
resultCaching: false,
}).restore(cloneDeep(data)),
),
initialDataForCaches.map(data =>
new InMemoryCache({
addTypename: false,
freezeResults: true,
}).restore(cloneDeep(data)),
),
];

cachesList.forEach((caches, i) => {
Expand All @@ -55,11 +49,6 @@ describe('Cache', () => {
...config,
resultCaching: false,
}),
new InMemoryCache({
addTypename: false,
...config,
freezeResults: true,
}),
];

caches.forEach((cache, i) => {
Expand Down
9 changes: 4 additions & 5 deletions packages/apollo-cache-inmemory/src/__tests__/roundtrip.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,6 @@ function assertDeeplyFrozen(value: any, stack: any[] = []) {

function storeRoundtrip(query: DocumentNode, result: any, variables = {}) {
const reader = new StoreReader();
const immutableReader = new StoreReader({ freezeResults: true });
const writer = new StoreWriter();

const store = writer.writeQueryToStore({
Expand All @@ -45,9 +44,9 @@ function storeRoundtrip(query: DocumentNode, result: any, variables = {}) {
expect(store).toBeInstanceOf(EntityCache);
expect(reader.readQueryFromStore(readOptions)).toBe(reconstructedResult);

const immutableResult = immutableReader.readQueryFromStore(readOptions);
const immutableResult = reader.readQueryFromStore(readOptions);
expect(immutableResult).toEqual(reconstructedResult);
expect(immutableReader.readQueryFromStore(readOptions)).toBe(immutableResult);
expect(reader.readQueryFromStore(readOptions)).toBe(immutableResult);
if (process.env.NODE_ENV !== 'production') {
try {
// Note: this illegal assignment will only throw in strict mode, but that's
Expand Down Expand Up @@ -234,8 +233,8 @@ describe('roundtrip', () => {
},
);

// Just because we read from the store using { freezeResults: true }, the
// original data should not be frozen.
// Reading immutable results from the store does not mean the original
// data should get frozen.
expect(Object.isExtensible(updateClub)).toBe(true);
expect(Object.isFrozen(updateClub)).toBe(false);
});
Expand Down
37 changes: 37 additions & 0 deletions packages/apollo-cache-inmemory/src/__tests__/writeToStore.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@ import { StoreWriter } from '../writeToStore';
import { defaultNormalizedCacheFactory } from '../entityCache';

import { makeReference } from '../helpers';
import { InMemoryCache } from '../inMemoryCache';

export function withWarning(func: Function, regex?: RegExp) {
let message: string = null as never;
Expand Down Expand Up @@ -1862,4 +1863,40 @@ describe('writing to the store', () => {
},
});
});

it('should not deep-freeze scalar objects', () => {
const query = gql`
query {
scalarFieldWithObjectValue
}
`;

const scalarObject = {
a: 1,
b: [2, 3],
c: {
d: 4,
e: 5,
},
};

const cache = new InMemoryCache();

cache.writeQuery({
query,
data: {
scalarFieldWithObjectValue: scalarObject,
},
});

expect(Object.isFrozen(scalarObject)).toBe(false);
expect(Object.isFrozen(scalarObject.b)).toBe(false);
expect(Object.isFrozen(scalarObject.c)).toBe(false);

const result = cache.readQuery<any>({ query });
expect(result.scalarFieldWithObjectValue).not.toBe(scalarObject);
expect(Object.isFrozen(result.scalarFieldWithObjectValue)).toBe(true);
expect(Object.isFrozen(result.scalarFieldWithObjectValue.b)).toBe(true);
expect(Object.isFrozen(result.scalarFieldWithObjectValue.c)).toBe(true);
});
});
3 changes: 0 additions & 3 deletions packages/apollo-cache-inmemory/src/inMemoryCache.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,14 +20,12 @@ import { KeyTrie } from 'optimism';

export interface InMemoryCacheConfig extends ApolloReducerConfig {
resultCaching?: boolean;
freezeResults?: boolean;
}

const defaultConfig: InMemoryCacheConfig = {
dataIdFromObject: defaultDataIdFromObject,
addTypename: true,
resultCaching: true,
freezeResults: false,
};

export function defaultDataIdFromObject(result: any): string | null {
Expand Down Expand Up @@ -92,7 +90,6 @@ export class InMemoryCache extends ApolloCache<NormalizedCacheObject> {

this.storeReader = new StoreReader({
cacheKeyRoot: this.cacheKeyRoot,
freezeResults: config.freezeResults,
possibleTypes: this.possibleTypes,
});

Expand Down
12 changes: 3 additions & 9 deletions packages/apollo-cache-inmemory/src/readFromStore.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -98,17 +98,14 @@ type ExecSubSelectedArrayOptions = {
type PossibleTypes = import('./inMemoryCache').InMemoryCache['possibleTypes'];
export interface StoreReaderConfig {
cacheKeyRoot?: KeyTrie<object>;
freezeResults?: boolean;
possibleTypes?: PossibleTypes;
}

export class StoreReader {
private freezeResults: boolean;
private possibleTypes?: PossibleTypes;

constructor({
cacheKeyRoot = new KeyTrie<object>(canUseWeakMap),
freezeResults = false,
possibleTypes,
}: StoreReaderConfig = {}) {
const {
Expand All @@ -117,7 +114,6 @@ export class StoreReader {
executeSubSelectedArray,
} = this;

this.freezeResults = freezeResults;
this.possibleTypes = possibleTypes;

this.executeStoreQuery = wrap((options: ExecStoreQueryOptions) => {
Expand Down Expand Up @@ -391,7 +387,7 @@ export class StoreReader {
// defensive shallow copies than necessary.
finalResult.result = mergeDeepArray(objectsToMerge);

if (this.freezeResults && process.env.NODE_ENV !== 'production') {
if (process.env.NODE_ENV !== 'production') {
Object.freeze(finalResult.result);
}

Expand Down Expand Up @@ -437,9 +433,7 @@ export class StoreReader {
if (!field.selectionSet) {
if (process.env.NODE_ENV !== 'production') {
assertSelectionSetForIdValue(contextValue.store, field, readStoreResult.result);
if (this.freezeResults) {
maybeDeepFreeze(readStoreResult);
}
maybeDeepFreeze(readStoreResult);
}
return readStoreResult;
}
Expand Down Expand Up @@ -525,7 +519,7 @@ export class StoreReader {
return item;
});

if (this.freezeResults && process.env.NODE_ENV !== 'production') {
if (process.env.NODE_ENV !== 'production') {
Object.freeze(array);
}

Expand Down
Loading