Skip to content

Commit

Permalink
Merge pull request #901 from apollostack/array-id-value
Browse files Browse the repository at this point in the history 
Make storing IDs in arrays consistent with the rest of the store
  • Loading branch information
@helfer
helfer authored Nov 15, 2016
2 parents 3a60f0d + 030cf74 commit 6de063c
Show file tree
Hide file tree
Showing 8 changed files with 95 additions and 38 deletions.
29 changes: 24 additions & 5 deletions src/data/mutationResults.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -122,6 +122,7 @@ function mutationResultArrayInsertReducer(state: NormalizedCache, {
});

// OK, now we need to get a dataID to pass to writeSelectionSetToStore
// XXX Set 'generated' flag?
const dataId = config.dataIdFromObject(scopedResult) || generateMutationResultDataId();

// Step 2: insert object into store with writeSelectionSet
Expand All @@ -145,10 +146,12 @@ function mutationResultArrayInsertReducer(state: NormalizedCache, {
path: restStorePath,
});

const idValue = { type: 'id', generated: false, id: dataId };

if (where === 'PREPEND') {
array.unshift(dataId);
array.unshift(idValue);
} else if (where === 'APPEND') {
array.push(dataId);
array.push(idValue);
} else {
throw new Error('Unsupported "where" option to ARRAY_INSERT.');
}
Expand Down Expand Up @@ -191,7 +194,7 @@ function removeRefsFromStoreObj(storeObj: any, dataId: any) {
let affected = false;

const cleanedObj = mapValues(storeObj, (value: any) => {
if (value === dataId) {
if (value && value.id === dataId) {
affected = true;
return null;
}
Expand Down Expand Up @@ -240,7 +243,7 @@ export function cleanArray(originalArray: any[], dataId: any): any[] {

return filteredArray;
} else {
const filteredArray = originalArray.filter((item) => item !== dataId);
const filteredArray = originalArray.filter((item) => item.id !== dataId);

if (filteredArray.length === originalArray.length) {
// No items were removed, return original array
Expand All @@ -267,7 +270,23 @@ function mutationResultArrayDeleteReducer(state: NormalizedCache, {
path: restStorePath,
});

array.splice(array.indexOf(dataId), 1);
// Find the index that matches the dataId
let index = -1;
array.some((item: any, i: number) => {
if (item && item.id === dataId) {
index = i;
return true;
}

return false;
});

if (index === -1) {
// We didn't have anything to remove, just return the same thing we had before
return state;
}

array.splice(index, 1);

return assign(state, {
[dataIdOfObj]: clonedObj,
Expand Down
31 changes: 23 additions & 8 deletions src/data/readFromStore.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@ import {
NormalizedCache,
isJsonValue,
isIdValue,
IdValue,
} from './storeUtils';

import {
Expand Down Expand Up @@ -73,11 +74,13 @@ type ReadStoreContext = {
let haveWarned = false;

const fragmentMatcher: FragmentMatcher = (
objId: string,
idValue: IdValue,
typeCondition: string,
context: ReadStoreContext
): boolean => {
const obj = context.store[objId];
assertIdValue(idValue);

const obj = context.store[idValue.id];

if (! obj) {
return false;
Expand Down Expand Up @@ -115,10 +118,13 @@ match fragments.`);

const readStoreResolver: Resolver = (
fieldName: string,
objId: string,
idValue: IdValue,
args: any,
context: ReadStoreContext
) => {
assertIdValue(idValue);

const objId = idValue.id;
const obj = context.store[objId];
const storeKeyName = storeKeyNameFromFieldNameAndArgs(fieldName, args);
const fieldValue = (obj || {})[storeKeyName];
Expand All @@ -139,10 +145,6 @@ Perhaps you want to use the \`returnPartialData\` option?`);
return fieldValue.json;
}

if (isIdValue(fieldValue)) {
return fieldValue.id;
}

return fieldValue;
};

Expand Down Expand Up @@ -171,7 +173,12 @@ export function diffQueryAgainstStore({
hasMissingField: false,
};

const result = graphqlAnywhere(readStoreResolver, query, 'ROOT_QUERY', context, variables, {
const rootIdValue = {
type: 'id',
id: 'ROOT_QUERY',
};

const result = graphqlAnywhere(readStoreResolver, query, rootIdValue, context, variables, {
fragmentMatcher,
});

Expand All @@ -180,3 +187,11 @@ export function diffQueryAgainstStore({
isMissing: context.hasMissingField,
};
}

function assertIdValue(idValue: IdValue) {
if (! isIdValue(idValue)) {
throw new Error(`Encountered a sub-selection on the query, but the store doesn't have \
an object reference. This should never happen during normal use unless you have custom code \
that is directly manipulating the store; please file an issue.`);
}
}
11 changes: 10 additions & 1 deletion src/data/writeToStore.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -350,11 +350,14 @@ function processArrayValue(
return processArrayValue(item, itemDataId, selectionSet, context);
}

let generated = true;

if (context.dataIdFromObject) {
const semanticId = context.dataIdFromObject(item);

if (semanticId) {
itemDataId = semanticId;
generated = false;
}
}

Expand All @@ -365,6 +368,12 @@ function processArrayValue(
context,
});

return itemDataId;
const idStoreValue: IdValue = {
type: 'id',
id: itemDataId,
generated,
};

return idStoreValue;
});
}
6 changes: 5 additions & 1 deletion test/client.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -458,7 +458,11 @@ describe('client', () => {
name: 'Luke Skywalker',
},
'ROOT_QUERY.allPeople({"first":1})': {
people: [ 'ROOT_QUERY.allPeople({"first":"1"}).people.0' ],
people: [ {
type: 'id',
generated: true,
id: 'ROOT_QUERY.allPeople({"first":"1"}).people.0',
} ],
},
ROOT_QUERY: {
'allPeople({"first":1})': {
Expand Down
12 changes: 6 additions & 6 deletions test/mutationResults.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -561,17 +561,17 @@ describe('mutation results', () => {
});
});

describe('array cleaning for ARRAY_DELETE', () => {
describe('array cleaning for DELETE behavior', () => {
it('maintains reference on flat array', () => {
const array = [1, 2, 3, 4, 5];
const array = [1, 2, 3, 4, 5].map(x => ({id: x}));
assert.isTrue(cleanArray(array, 6) === array);
assert.isFalse(cleanArray(array, 3) === array);
});

it('works on nested array', () => {
const array = [
[1, 2, 3, 4, 5],
[6, 7, 8, 9, 10],
[1, 2, 3, 4, 5].map(x => ({id: x})),
[6, 7, 8, 9, 10].map(x => ({id: x})),
];

const cleaned = cleanArray(array, 5);
Expand All @@ -581,8 +581,8 @@ describe('mutation results', () => {

it('maintains reference on nested array', () => {
const array = [
[1, 2, 3, 4, 5],
[6, 7, 8, 9, 10],
[1, 2, 3, 4, 5].map(x => ({id: x})),
[6, 7, 8, 9, 10].map(x => ({id: x})),
];

assert.isTrue(cleanArray(array, 11) === array);
Expand Down
20 changes: 14 additions & 6 deletions test/optimistic.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -564,8 +564,8 @@ describe('optimistic mutation results', () => {
assert.equal((dataInStore['TodoList5'] as any).todos.length, 4);
assert.notProperty(dataInStore, 'Todo99');
assert.property(dataInStore, 'Todo66');
assert.include((dataInStore['TodoList5'] as any).todos, 'Todo66');
assert.notInclude((dataInStore['TodoList5'] as any).todos, 'Todo99');
assert.include((dataInStore['TodoList5'] as any).todos, realIdValue('Todo66'));
assert.notInclude((dataInStore['TodoList5'] as any).todos, realIdValue('Todo99'));
});
});
it('can run 2 mutations concurrently and handles all intermediate states well', () => {
Expand All @@ -574,8 +574,8 @@ describe('optimistic mutation results', () => {
assert.equal((dataInStore['TodoList5'] as any).todos.length, 5);
assert.property(dataInStore, 'Todo99');
assert.property(dataInStore, 'Todo66');
assert.include((dataInStore['TodoList5'] as any).todos, 'Todo66');
assert.include((dataInStore['TodoList5'] as any).todos, 'Todo99');
assert.include((dataInStore['TodoList5'] as any).todos, realIdValue('Todo66'));
assert.include((dataInStore['TodoList5'] as any).todos, realIdValue('Todo99'));
assert.equal((dataInStore['Todo99'] as any).text, expectedText1);
assert.equal((dataInStore['Todo66'] as any).text, expectedText2);
}
Expand Down Expand Up @@ -839,8 +839,8 @@ describe('optimistic mutation results', () => {
assert.equal((dataInStore['TodoList5'] as any).todos.length, 4);
assert.notProperty(dataInStore, 'Todo99');
assert.property(dataInStore, 'Todo66');
assert.include((dataInStore['TodoList5'] as any).todos, 'Todo66');
assert.notInclude((dataInStore['TodoList5'] as any).todos, 'Todo99');
assert.include((dataInStore['TodoList5'] as any).todos, realIdValue('Todo66'));
assert.notInclude((dataInStore['TodoList5'] as any).todos, realIdValue('Todo99'));
});
});
});
Expand Down Expand Up @@ -1170,3 +1170,11 @@ describe('optimistic mutation - githunt comments', () => {
});
});
});

function realIdValue(id: string) {
return {
type: 'id',
generated: false,
id,
};
}
10 changes: 4 additions & 6 deletions test/readFromStore.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -179,7 +179,6 @@ describe('reading from the store', () => {
nestedObj: {
type: 'id',
id: 'abcde',
nullField: null,
generated: false,
},
}) as StoreObject,
Expand All @@ -188,7 +187,6 @@ describe('reading from the store', () => {
type: 'id',
id: 'abcdef',
generated: false,
nullField: null,
},
}) as StoreObject,
abcdef: result.deepNestedObj as StoreObject,
Expand Down Expand Up @@ -270,8 +268,8 @@ describe('reading from the store', () => {
const store = {
'ROOT_QUERY': _.assign({}, _.assign({}, _.omit(result, 'nestedArray')), {
nestedArray: [
'abcd.nestedArray.0',
'abcd.nestedArray.1',
{ type: 'id', generated: true, id: 'abcd.nestedArray.0' },
{ type: 'id', generated: true, id: 'abcd.nestedArray.1' },
],
}) as StoreObject,
'abcd.nestedArray.0': result.nestedArray[0],
Expand Down Expand Up @@ -329,7 +327,7 @@ describe('reading from the store', () => {
'ROOT_QUERY': _.assign({}, _.assign({}, _.omit(result, 'nestedArray')), {
nestedArray: [
null,
'abcd.nestedArray.1',
{ type: 'id', generated: true, id: 'abcd.nestedArray.1' },
],
}) as StoreObject,
'abcd.nestedArray.1': result.nestedArray[1],
Expand Down Expand Up @@ -384,7 +382,7 @@ describe('reading from the store', () => {
'ROOT_QUERY': _.assign({}, _.assign({}, _.omit(result, 'nestedArray')), {
nestedArray: [
null,
'abcde',
{ type: 'id', generated: false, id: 'abcde' },
],
}) as StoreObject,
'abcde': result.nestedArray[1],
Expand Down
14 changes: 9 additions & 5 deletions test/writeToStore.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -332,7 +332,11 @@ describe('writing to the store', () => {
dataIdFromObject: getIdField,
}), {
'ROOT_QUERY': _.assign({}, _.assign({}, _.omit(result, 'nestedArray')), {
nestedArray: result.nestedArray.map(_.property('id')),
nestedArray: result.nestedArray.map((obj: any) => ({
type: 'id',
id: obj.id,
generated: false,
})),
}),
[result.nestedArray[0].id]: result.nestedArray[0],
[result.nestedArray[1].id]: result.nestedArray[1],
Expand Down Expand Up @@ -378,7 +382,7 @@ describe('writing to the store', () => {
}), {
'ROOT_QUERY': _.assign({}, _.assign({}, _.omit(result, 'nestedArray')), {
nestedArray: [
result.nestedArray[0].id,
{ type: 'id', id: result.nestedArray[0].id, generated: false },
null,
],
}),
Expand Down Expand Up @@ -428,8 +432,8 @@ describe('writing to the store', () => {
assert.deepEqual(normalized, {
'ROOT_QUERY': _.assign({}, _.assign({}, _.omit(result, 'nestedArray')), {
nestedArray: [
`ROOT_QUERY.nestedArray.0`,
`ROOT_QUERY.nestedArray.1`,
{ type: 'id', generated: true, id: `ROOT_QUERY.nestedArray.0` },
{ type: 'id', generated: true, id: `ROOT_QUERY.nestedArray.1` },
],
}),
[`ROOT_QUERY.nestedArray.0`]: result.nestedArray[0],
Expand Down Expand Up @@ -476,7 +480,7 @@ describe('writing to the store', () => {
'ROOT_QUERY': _.assign({}, _.assign({}, _.omit(result, 'nestedArray')), {
nestedArray: [
null,
`ROOT_QUERY.nestedArray.1`,
{ type: 'id', generated: true, id: `ROOT_QUERY.nestedArray.1` },
],
}),
[`ROOT_QUERY.nestedArray.1`]: result.nestedArray[1],
Expand Down

0 comments on commit 6de063c

Please sign in to comment.