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

Support for @skip and @include #275

Merged
merged 21 commits into from
Jun 21, 2016
Merged
Show file tree
Hide file tree
Changes from 7 commits
Commits
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
2 changes: 2 additions & 0 deletions src/QueryManager.ts
Original file line number Diff line number Diff line change
Expand Up @@ -401,6 +401,8 @@ export class QueryManager {
let resultFromStore;
try {
// ensure result is combined with data already in store
// this will throw an error if there are missing fields that were
// included (e.g. @skip was false).
resultFromStore = readSelectionSetFromStore({
store: this.getApolloState().data,
rootId: querySS.id,
Expand Down
45 changes: 28 additions & 17 deletions src/data/diffAgainstStore.ts
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,10 @@ import {
FragmentMap,
} from '../queries/getFromAST';

import {
shouldInclude,
} from '../queries/directives';

export interface DiffResult {
result: any;
isMissing?: 'true';
Expand Down Expand Up @@ -120,6 +124,8 @@ export function diffSelectionSetAgainstStore({
selectionSet.selections.forEach((selection) => {
// Don't push more than one missing field per field in the query
let missingFieldPushed = false;
const included = shouldInclude(selection, variables);
Copy link
Contributor

@helfer helfer Jun 17, 2016

Choose a reason for hiding this comment

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

move this definition down to just before it is used.
and call it something other than included, maybe includeField


function pushMissingField(missingField: Selection) {
if (!missingFieldPushed) {
missingFields.push(missingField);
Expand All @@ -128,25 +134,30 @@ export function diffSelectionSetAgainstStore({
}

if (isField(selection)) {
const {
result: fieldResult,
isMissing: fieldIsMissing,
} = diffFieldAgainstStore({
field: selection,
throwOnMissingField,
variables,
rootId,
store,
fragmentMap,
});

if (fieldIsMissing) {
pushMissingField(selection);
if (included) {
const {
result: fieldResult,
isMissing: fieldIsMissing,
} = diffFieldAgainstStore({
field: selection,
throwOnMissingField,
variables,
rootId,
store,
fragmentMap,
});

if (fieldIsMissing) {
pushMissingField(selection);
} else {
const resultFieldKey = resultKeyNameFromField(selection);

result[resultFieldKey] = fieldResult;
}
} else {
const resultFieldKey = resultKeyNameFromField(selection);

result[resultFieldKey] = fieldResult;
pushMissingField(selection);
}

} else if (isInlineFragment(selection)) {
const {
result: fieldResult,
Expand Down
32 changes: 22 additions & 10 deletions src/data/writeToStore.ts
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,10 @@ import {
IdGetter,
} from './extensions';

import {
shouldInclude,
} from '../queries/directives';

// import {
// printAST,
// } from './debug';
Expand Down Expand Up @@ -130,24 +134,32 @@ export function writeSelectionSetToStore({
//to us for the fragments.
fragmentMap = {};
}

selectionSet.selections.forEach((selection) => {
if (isField(selection)) {
const resultFieldKey: string = resultKeyNameFromField(selection);
const value: any = result[resultFieldKey];
const included = shouldInclude(selection, variables);

if (isUndefined(value)) {
if (isUndefined(value) && included) {
throw new Error(`Can't find field ${resultFieldKey} on result object ${dataId}.`);
}

writeFieldToStore({
dataId,
value,
variables,
store,
field: selection,
dataIdFromObject,
fragmentMap,
});
if (!isUndefined(value) && !included) {
throw new Error(`Found extra field ${resultFieldKey} on result object ${dataId}.`);
}

if (!isUndefined(value)) {
writeFieldToStore({
dataId,
value,
variables,
store,
field: selection,
dataIdFromObject,
fragmentMap,
});
}
} else if (isInlineFragment(selection)) {
// XXX what to do if this tries to write the same fields? Also, type conditions...
writeSelectionSetToStore({
Expand Down
65 changes: 65 additions & 0 deletions src/queries/directives.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,65 @@
// Provides the methods that allow QueryManager to handle
// the `skip` and `include` directives within GraphQL.
import {
Selection,
Variable,
BooleanValue,
Directive,
} from 'graphql';
Copy link
Contributor

Choose a reason for hiding this comment

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

Does this import all of graphql now, because the exact path isn't specified?

Copy link
Contributor

Choose a reason for hiding this comment

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

These are just the types, it doesn't actually import anything afaik


export function evaluateSkipInclude(directive: Directive, variables: { [name: string]: any }): Boolean {
//evaluate the "if" argument and skip (i.e. return undefined) if it evaluates to true.
const directiveArguments = directive.arguments;
if (directiveArguments.length !== 1) {
throw new Error(`Incorrect number of arguments for the @$(directiveName} directive.`);
}

const directiveName = directive.name.value;
const ifArgument = directive.arguments[0];
Copy link
Contributor

Choose a reason for hiding this comment

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

It's good to do sanity checks, but I'm not sure they're necessary in this case. We might also just delegate that to GraphQL validation, which I'm pretty sure apollo client will include at some point in the future. For the time being, I think it's fine not to provide great error messages, because it will simplify the code a lot.

if (!ifArgument.name || ifArgument.name.value !== 'if') {
throw new Error(`Invalid argument for the @${directiveName} directive.`);
}

const ifValue = directive.arguments[0].value;
let evaledValue: Boolean = false;
if (!ifValue || ifValue.kind !== 'BooleanValue') {
// means it has to be a variable value if this is a valid @skip or @include directive
if (ifValue.kind !== 'Variable') {
throw new Error(`Invalid argument value for the @${directiveName} directive.`);
} else {
evaledValue = variables[(ifValue as Variable).name.value];
if (evaledValue === undefined) {
throw new Error(`Invalid variable referenced in @${directiveName} directive.`);
}
}
} else {
evaledValue = (ifValue as BooleanValue).value;
}

if (directiveName === 'skip') {
evaledValue = !evaledValue;
}

return evaledValue;
}

export function shouldInclude(selection: Selection, variables?: { [name: string]: any }): Boolean {
if (!variables) {
variables = {};
}

if (!selection.directives) {
return true;
}

let res: Boolean = true;
selection.directives.map((directive) => {
Copy link
Contributor

Choose a reason for hiding this comment

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

You don't need a map here. graphql-js assumes that there's only one skip or one include directive, so we can do the same.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think the spec actually defines the behavior that should occur when we have both a skip and include directive here. Since it does, I think it makes sense to include handling that case.

Copy link
Contributor

Choose a reason for hiding this comment

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

I meant that there won't be two skips or two includes, at most one of each.

if (directive.name.value !== 'skip' && directive.name.value !== 'include') {
throw new Error(`Directive ${directive.name.value} not supported.`);
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 this should just ignore any directives that are not skip or include, not throw an error.

}
if (!evaluateSkipInclude(directive, variables)) {
res = false;
}
});
return res;
}
10 changes: 9 additions & 1 deletion src/store.ts
Original file line number Diff line number Diff line change
Expand Up @@ -72,21 +72,29 @@ export function createApolloStore({
reduxRootKey = 'apollo',
initialState,
config = {},
reportCrashes,
}: {
reduxRootKey?: string,
initialState?: any,
config?: ApolloReducerConfig,
reportCrashes?: boolean,
} = {}): ApolloStore {
const enhancers = [];

if (reportCrashes === undefined) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I had to add this because otherwise one of the unit tests would print all of this useless stuff every time it ran because it irked the crash reporter.

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure I understand, can you show me later?

Copy link
Contributor

Choose a reason for hiding this comment

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

Use the TypeScript default argument syntax, reportCrashes = true

reportCrashes = true;
}

if (typeof window !== 'undefined') {
const anyWindow = window as any;
if (anyWindow.devToolsExtension) {
enhancers.push(anyWindow.devToolsExtension());
}
}

enhancers.push(applyMiddleware(crashReporter));
if (reportCrashes) {
enhancers.push(applyMiddleware(crashReporter));
}

return createStore(
combineReducers({ [reduxRootKey]: createApolloReducer(config) }),
Expand Down
91 changes: 91 additions & 0 deletions test/client.ts
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,14 @@ import {
applyMiddleware,
} from 'redux';

import {
createApolloStore,
} from '../src/store';

import {
QueryManager,
} from '../src/QueryManager';

import {
createNetworkInterface,
HTTPNetworkInterface,
Expand Down Expand Up @@ -707,6 +715,89 @@ describe('client', () => {
});
});

describe('directives', () => {
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 can move these tests to roundtrip.ts, they will be much more concise there. No need to exercise querymanager for all of them, IMO.

it('should be able to send a query with a skip directive true', (done) => {
const query = gql`
query {
fortuneCookie @skip(if: true)
}`;
const result = {};
const networkInterface = mockNetworkInterface(
{
request: { query },
result: { data: result },
}
);
const client = new ApolloClient({
networkInterface,
});
client.query({ query }).then((actualResult) => {
assert.deepEqual(actualResult.data, result);
done();
});
});

it('should be able to send a query with a skip directive false', (done) => {
const query = gql`
query {
fortuneCookie @skip(if: false)
}`;
const result = {
fortuneCookie: 'result',
};
const networkInterface = mockNetworkInterface(
{
request: { query },
result: { data: result },
}
);
const client = new ApolloClient({
networkInterface,
});
client.query({ query }).then((actualResult) => {
assert.deepEqual(actualResult.data, result);
done();
});
});

it('should reject the query promise if skipped data arrives in the result', (done) => {
const query = gql`
query {
fortuneCookie @skip(if: true)
otherThing
}`;
const result = {
fortuneCookie: 'you will go far',
otherThing: 'false',
};
const networkInterface = mockNetworkInterface(
{
request: { query },
result: { data: result },
}
);
const client = new ApolloClient({
networkInterface,
});
// we need this so it doesn't print out a bunch of stuff we don't need
// when we're trying to test an exception.
client.store = createApolloStore({ reportCrashes: false });
client.queryManager = new QueryManager({
networkInterface,
store: client.store,
reduxRootKey: 'apollo',
});

client.query({ query }).then(() => {
// do nothing
}).catch((error) => {
assert.include(error.message, 'Found extra field');
done();
});

});
});

describe('accepts dataIdFromObject option', () => {
const query = gql`
query people {
Expand Down
Loading