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 15 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
1 change: 1 addition & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ Expect active development and potentially significant breaking changes in the `0

### vNEXT
- Added name fragment support within mutations [Issue #273](https://github.com/apollostack/apollo-client/issues/273) and [PR #274](https://github.com/apollostack/apollo-client/pull/274)
- Added support for `@skip` and `@include` directives - see [Issue #237](https://github.com/apollostack/apollo-client/issues/237) and [PR #275](https://github.com/apollostack/apollo-client/pull/275)
- Now sending the operation name along with the query to the server [Issue #259](https://github.com/apollostack/apollo-client/issues/259) and [PR #282](https://github.com/apollostack/apollo-client/pull/282)

### v0.3.13
Expand Down
2 changes: 2 additions & 0 deletions src/QueryManager.ts
Original file line number Diff line number Diff line change
Expand Up @@ -399,6 +399,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 in
// the results if returnPartialData is false.
resultFromStore = readSelectionSetFromStore({
store: this.getApolloState().data,
rootId: querySS.id,
Expand Down
41 changes: 28 additions & 13 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,7 @@ export function diffSelectionSetAgainstStore({
selectionSet.selections.forEach((selection) => {
// Don't push more than one missing field per field in the query
let missingFieldPushed = false;

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

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

if (fieldIsMissing) {
// even if the field is not included, we want to keep it in the
// query that is sent to the server. So, we push it into the set of
// fields that is missing.
pushMissingField(selection);
} else {
} else if (includeField) {
const resultFieldKey = resultKeyNameFromField(selection);

result[resultFieldKey] = fieldResult;
Expand Down Expand Up @@ -231,19 +241,24 @@ function diffFieldAgainstStore({
rootId,
store,
fragmentMap,
included,
Copy link
Contributor

Choose a reason for hiding this comment

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

Actually this can just be included = true

}: {
field: Field,
throwOnMissingField: boolean,
variables: Object,
rootId: string,
store: NormalizedCache,
fragmentMap?: FragmentMap,
included?: Boolean,
}): FieldDiffResult {
const storeObj = store[rootId] || {};
const storeFieldKey = storeKeyNameFromField(field, variables);
if (included === undefined) {
Copy link
Contributor

Choose a reason for hiding this comment

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

This should be typeof included === 'undefined'.

Copy link
Contributor

Choose a reason for hiding this comment

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

We don't need this check at all if we use typescript default arg syntax

included = true;
}

if (! has(storeObj, storeFieldKey)) {
if (throwOnMissingField) {
if (throwOnMissingField && included) {
throw new Error(`Can't find field ${storeFieldKey} on object ${storeObj}.`);
}

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,
} 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 shouldInclude(selection: Selection, variables?: { [name: string]: any }): Boolean {
if (!variables) {
variables = {};
}

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

let res: Boolean = true;
selection.directives.forEach((directive) => {
// TODO should move this validation to GraphQL validation once that's implemented.
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think we should ever validate on the client, but maybe as a static analysis step.

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 client-side validation is going to be a thing no matter what. Maybe not in the next 6 months, but it's certainly a useful feature we should eventually have.

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't see why we would want client-side validation at runtime, that seems like just adding unnecessary code to the client.

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't mean validation that you can do at compile-time, but validation on input values etc., which I think we'll add through schema directives.

Copy link
Contributor

Choose a reason for hiding this comment

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

Hmm, so I guess given the variables and the query we'll try to figure out if they work, and if not don't send the query? I guess that could be good, so that you can save on server resources. I would be a bit worried about validation being inconsistent on client and server but we can cross that bridge when we get there.

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.

}

//evaluate the "if" argument and skip (i.e. return undefined) if it evaluates to true.
const directiveArguments = directive.arguments;
const directiveName = directive.name.value;
if (directiveArguments.length !== 1) {
throw new Error(`Incorrect number of arguments for the @${directiveName} directive.`);
}


const ifArgument = directive.arguments[0];
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.`);
Copy link
Contributor

Choose a reason for hiding this comment

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

This error could be more informative if it said "argument for the @Skip directive must be a variable"

} else {
evaledValue = variables[(ifValue as Variable).name.value];
if (evaledValue === undefined) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Same, don't === undefined.

Copy link
Contributor

Choose a reason for hiding this comment

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

You could use variables.hasOwnProperty('name')

throw new Error(`Invalid variable referenced in @${directiveName} directive.`);
}
}
} else {
evaledValue = (ifValue as BooleanValue).value;
}

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

if (!evaledValue) {
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
90 changes: 90 additions & 0 deletions test/client.ts
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,14 @@ import {
applyMiddleware,
} from 'redux';

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

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

import {
createNetworkInterface,
HTTPNetworkInterface,
Expand Down Expand Up @@ -710,6 +718,88 @@ 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();
});
});
});

it('should send operationName along with the query to the server', (done) => {
const query = gql`
query myQueryName {
Expand Down
Loading