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

Specifically handle the connection directive when selecting store keys, fix #1779 #1801

Merged
merged 9 commits into from
Jun 24, 2017
1 change: 1 addition & 0 deletions AUTHORS
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,7 @@ Robert Dickert <robert.dickert@gmail.com>
Robin Ricard <ricard.robin@gmail.com>
Sashko Stubailo <s.stubailo@gmail.com>
Sashko Stubailo <sashko@stubailo.com>
Shadaj Laddad <shadaj@meteor.com>
Simon Tucker <srtucker22@gmail.com>
Slava Kim <imslavko@gmail.com>
Slava Kim <slv@mit.edu>
Expand Down
2 changes: 2 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,8 @@
### 1.5.0
- `batchInterval` now has a default value of 10 ms [PR #1793](https://github.com/apollographql/apollo-client/pull/1793)
- Added `batchMax` to allow you to limit the amount of queries in one batch. [PR #1659](https://github.com/apollographql/apollo-client/pull/1659)
- the `@connection(key: ...)` directive can now be used to specify the key to use
for the Apollo store and is removed by default when sending queries to the server [PR #1801](https://github.com/apollographql/apollo-client/pull/1801)

### 1.4.2
- Improved error messages for writeToStore, readFragment and writeFragment [PR #1766](https://github.com/apollographql/apollo-client/pull/1766), [PR #1722](https://github.com/apollographql/apollo-client/pull/1722)
Expand Down
4 changes: 2 additions & 2 deletions src/ApolloClient.ts
Original file line number Diff line number Diff line change
Expand Up @@ -65,7 +65,7 @@ import {
} from './core/watchQueryOptions';

import {
storeKeyNameFromFieldNameAndArgs,
getStoreKeyName,
} from './data/storeUtils';

import {
Expand Down Expand Up @@ -222,7 +222,7 @@ export default class ApolloClient implements DataProxy {
this.disableNetworkFetches = ssrMode || ssrForceFetchDelay > 0;
this.dataId = dataIdFromObject = dataIdFromObject || defaultDataIdFromObject;
this.dataIdFromObject = this.dataId;
this.fieldWithArgs = storeKeyNameFromFieldNameAndArgs;
this.fieldWithArgs = getStoreKeyName;
this.queryDeduplication = queryDeduplication;
this.ssrMode = ssrMode;

Expand Down
40 changes: 23 additions & 17 deletions src/core/QueryManager.ts
Original file line number Diff line number Diff line change
Expand Up @@ -1096,24 +1096,30 @@ export class QueryManager {
}

return result;
}).then(() => {

}).then((result) => {
let resultFromStore: any;
try {
// ensure result is combined with data already in store
// this will throw an error if there are missing fields in
// the results if returnPartialData is false.
resultFromStore = readQueryFromStore({
store: this.getApolloState().data,
variables,
query: document,
config: this.reducerConfig,
fragmentMatcherFunction: this.fragmentMatcher.match,
});
// ensure multiple errors don't get thrown
/* tslint:disable */
} catch (e) {}
/* tslint:enable */

if (fetchMoreForQueryId) {
// XXX We don't write fetchMore results to the store because this would overwrite
// the original result in case an @connection directive is used.
resultFromStore = result.data;
} else {
try {
// ensure result is combined with data already in store
// this will throw an error if there are missing fields in
// the results if returnPartialData is false.
resultFromStore = readQueryFromStore({
store: this.getApolloState().data,
variables,
query: document,
config: this.reducerConfig,
fragmentMatcherFunction: this.fragmentMatcher.match,
});
// ensure multiple errors don't get thrown
/* tslint:disable */
} catch (e) {}
/* tslint:enable */
}

const { reducerError } = this.getApolloState();
if (reducerError && reducerError.queryId === queryId) {
Expand Down
6 changes: 3 additions & 3 deletions src/data/readFromStore.ts
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@ import {
} from './storeUtils';

import {
storeKeyNameFromFieldNameAndArgs,
getStoreKeyName,
} from './storeUtils';

import {
Expand Down Expand Up @@ -132,13 +132,13 @@ const readStoreResolver: Resolver = (
idValue: IdValueWithPreviousResult,
args: any,
context: ReadStoreContext,
{ resultKey }: ExecInfo,
{ resultKey, directives }: ExecInfo,
) => {
assertIdValue(idValue);

const objId = idValue.id;
const obj = context.store[objId];
const storeKeyName = storeKeyNameFromFieldNameAndArgs(fieldName, args);
const storeKeyName = getStoreKeyName(fieldName, args, directives);
let fieldValue = (obj || {})[storeKeyName];

if (typeof fieldValue === 'undefined') {
Expand Down
3 changes: 2 additions & 1 deletion src/data/store.ts
Original file line number Diff line number Diff line change
Expand Up @@ -69,7 +69,8 @@ export function data(
// Ignore results from old requests
// XXX this means that if you have a refetch interval which is shorter than your roundtrip time,
// your query will be in the loading state forever!
if (action.requestId < queries[action.queryId].lastRequestId) {
// do not write to the store if this is for fetchMore
if (action.requestId < queries[action.queryId].lastRequestId || action.fetchMoreForQueryId) {
return previousState;
}

Expand Down
33 changes: 27 additions & 6 deletions src/data/storeUtils.ts
Original file line number Diff line number Diff line change
Expand Up @@ -74,19 +74,40 @@ export function valueToObjectRepresentation(argObj: any, name: NameNode, value:
}

export function storeKeyNameFromField(field: FieldNode, variables?: Object): string {
if (field.arguments && field.arguments.length) {
const argObj: Object = {};
let directivesObj: any = null;
if (field.directives) {
directivesObj = {};
field.directives.forEach((directive) => {
directivesObj[directive.name.value] = {};

if (directive.arguments) {
directive.arguments.forEach((({name, value}) => valueToObjectRepresentation(
directivesObj[directive.name.value], name, value, variables)));
}
});
}

let argObj: any = null;
if (field.arguments && field.arguments.length) {
argObj = {};
field.arguments.forEach(({name, value}) => valueToObjectRepresentation(
argObj, name, value, variables));

return storeKeyNameFromFieldNameAndArgs(field.name.value, argObj);
}

return field.name.value;
return getStoreKeyName(field.name.value, argObj, directivesObj);
}

export function storeKeyNameFromFieldNameAndArgs(fieldName: string, args?: Object): string {
export type Directives = {
[directiveName: string]: {
[argName: string]: any;
};
};

export function getStoreKeyName(fieldName: string, args?: Object, directives?: Directives): string {
if (directives && directives['connection'] && directives['connection']['key']) {
return directives['connection']['key'];
}

if (args) {
const stringifiedArgs: string = JSON.stringify(args);

Expand Down
43 changes: 43 additions & 0 deletions src/queries/queryTransform.ts
Original file line number Diff line number Diff line change
Expand Up @@ -51,6 +51,38 @@ function addTypenameToSelectionSet(
}
}

function removeConnectionDirectiveFromSelectionSet(selectionSet: SelectionSetNode) {
if (selectionSet.selections) {
selectionSet.selections.forEach((selection) => {
if (selection.kind === 'Field' && selection as FieldNode && selection.directives) {
selection.directives = selection.directives.filter((directive) => {
const willRemove = directive.name.value === 'connection';
if (willRemove) {
if (!directive.arguments || !directive.arguments.some((arg) => arg.name.value === 'key')) {
console.warn('Removing an @connection directive even though it does not have a key. ' +
'You may want to use the key parameter to specify a store key.');
}
}

return !willRemove;
});
}
});

selectionSet.selections.forEach((selection) => {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why not include the recursion inside the previous loop?

if (selection.kind === 'Field') {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we could just "duck type" here and check if selection.selectionSet exists, and if it does recurse on it.

if (selection.selectionSet) {
removeConnectionDirectiveFromSelectionSet(selection.selectionSet);
}
} else if (selection.kind === 'InlineFragment') {
if (selection.selectionSet) {
removeConnectionDirectiveFromSelectionSet(selection.selectionSet);
}
}
});
}
}

export function addTypenameToDocument(doc: DocumentNode) {
checkDocument(doc);
const docClone = cloneDeep(doc);
Expand All @@ -62,3 +94,14 @@ export function addTypenameToDocument(doc: DocumentNode) {

return docClone;
}

export function removeConnectionDirectiveFromDocument(doc: DocumentNode) {
checkDocument(doc);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since we're now doing all of these twice (for addTypename as well), it might make more sense to just have one function called applyTransformsToDocument and then have an addTypenameTransform and a removeConnectionDirectiveTransform. The applyTransformsToDocument function could then just clone the document at the beginning, and all the transforms could modify the clone in place. We'd also only have to check once.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Never mind, we won't do it in the same place, so this comment doesn't apply any more.

const docClone = cloneDeep(doc);

docClone.definitions.forEach((definition: DefinitionNode) => {
removeConnectionDirectiveFromSelectionSet((definition as OperationDefinitionNode).selectionSet);
});

return docClone;
}
10 changes: 10 additions & 0 deletions src/transport/networkInterface.ts
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,10 @@ import {
BatchAfterwareInterface,
} from './afterware';

import {
removeConnectionDirectiveFromDocument,
} from '../queries/queryTransform';

/**
* This is an interface that describes an GraphQL document to be sent
* to the server.
Expand Down Expand Up @@ -192,6 +196,12 @@ export class HTTPFetchNetworkInterface extends BaseNetworkInterface {
return this.applyMiddlewares({
request,
options,
}).then((rao) => {
if (rao.request.query) {
rao.request.query = removeConnectionDirectiveFromDocument(rao.request.query);
}

return rao;
}).then( (rao) => this.fetchFromRemoteEndpoint.call(this, rao))
.then(response => this.applyAfterwares({
response: response as Response,
Expand Down
129 changes: 129 additions & 0 deletions test/client.ts
Original file line number Diff line number Diff line change
Expand Up @@ -2554,8 +2554,137 @@ describe('client', () => {

return withWarning(() => client.query({ query }), /Missing field description/);
});

it('runs a query with the connection directive and writes it to the store key defined in the directive', () => {
const query = gql`
{
books(skip: 0, limit: 2) @connection(key: "abc") {
name
}
}`;

const transformedQuery = gql`
{
books(skip: 0, limit: 2) @connection(key: "abc") {
name
__typename
}
}`;

const result = {
'books': [
{
'name': 'abcd',
'__typename': 'Book',
},
],
};

const networkInterface = mockNetworkInterface({
request: { query: transformedQuery },
result: { data: result },
});

const client = new ApolloClient({
networkInterface,
});

return client.query({ query }).then((actualResult) => {
assert.deepEqual(actualResult.data, result);
});
});

it('should not remove the connection directive at the store level', () => {
const query = gql`
{
books(skip: 0, limit: 2) @connection {
name
}
}`;

const transformedQuery = gql`
{
books(skip: 0, limit: 2) @connection {
name
__typename
}
}`;

const result = {
'books': [
{
'name': 'abcd',
'__typename': 'Book',
},
],
};

const networkInterface = mockNetworkInterface({
request: { query: transformedQuery },
result: { data: result },
});

const client = new ApolloClient({
networkInterface,
});

return client.query({ query }).then((actualResult) => {
assert.deepEqual(actualResult.data, result);
});
});
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There should be an end-to-end test as well that checks that the store contains the right objects after a query. The structure can be the same as for this test.

Copy link
Contributor

@helfer helfer Jun 19, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Add a test to check if it prints a warning when @connection is used without the key argument. You can use withWarning or withError for that.

});

it('should run a query with the connection directive and write the result to the store key defined in the directive', () => {
const query = gql`
{
books(skip: 0, limit: 2) @connection(key: "abc") {
name
}
}`;

const transformedQuery = gql`
{
books(skip: 0, limit: 2) @connection(key: "abc") {
name
__typename
}
}`;

const result = {
'books': [
{
'name': 'abcd',
'__typename': 'Book',
},
],
};

const networkInterface = mockNetworkInterface({
request: { query: transformedQuery },
result: { data: result },
});

const client = new ApolloClient({
networkInterface,
});

return client.query({ query }).then((actualResult) => {
assert.deepEqual(actualResult.data, result);
assert.deepEqual(client.store.getState().apollo.data, {
'ROOT_QUERY.abc.0': { name: 'abcd', __typename: 'Book' },
'ROOT_QUERY': {
abc: [
{
'generated': true,
'id': 'ROOT_QUERY.abc.0',
'type': 'id',
},
],
},
});
});
});

function clientRoundtrip(
query: DocumentNode,
data: ExecutionResult,
Expand Down
Loading