Skip to content

Commit

Permalink
Minor @defer updates
Browse files Browse the repository at this point in the history
This address most of the comment from @glasser on apollographql#1958, namely:
- it removes/update some outdated TODOs.
- it fixup the definition of @defer/@stream to match current spec.
- makes serialized query plan for condition hopefully a tad easier to
  parse (solely impact unit tests readability).
- adds a test to ensure that if the same field is queries both "normally"
  and deferred, we do query it twice (that's what spec specifies).
  • Loading branch information
Sylvain Lebresne committed Aug 29, 2022
1 parent 36191f7 commit 5204412
Show file tree
Hide file tree
Showing 4 changed files with 463 additions and 317 deletions.
17 changes: 13 additions & 4 deletions internals-js/src/__tests__/definitions.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -817,17 +817,26 @@ test('correctly convert to a graphQL-js schema', () => {
expect(printGraphQLjsSchema(graphqQLSchema)).toMatchString(sdl);
});

test('Conversion to graphQL-js schema can optionally include @defer definition', () => {
test('Conversion to graphQL-js schema can optionally include @defer and/or @streams definition(s)', () => {
const sdl = `
type Query {
x: Int
}
`;
const schema = parseSchema(sdl);

const graphqQLSchema = schema.toGraphQLJSSchema({ includeDefer: true });
expect(printGraphQLjsSchema(graphqQLSchema)).toMatchString(`
directive @defer(label: String, if: Boolean) on FRAGMENT_SPREAD | INLINE_FRAGMENT
expect(printGraphQLjsSchema(schema.toGraphQLJSSchema({ includeDefer: true }))).toMatchString(`
directive @defer(label: String, if: Boolean! = true) on FRAGMENT_SPREAD | INLINE_FRAGMENT
type Query {
x: Int
}
`);

expect(printGraphQLjsSchema(schema.toGraphQLJSSchema({ includeDefer: true, includeStream: true }))).toMatchString(`
directive @defer(label: String, if: Boolean! = true) on FRAGMENT_SPREAD | INLINE_FRAGMENT
directive @stream(label: String, initialCount: Int = 0, if: Boolean! = true) on FIELD
type Query {
x: Int
Expand Down
18 changes: 10 additions & 8 deletions internals-js/src/definitions.ts
Original file line number Diff line number Diff line change
Expand Up @@ -1128,15 +1128,16 @@ const graphQLBuiltInDirectivesSpecifications: readonly DirectiveSpecification[]
locations: [DirectiveLocation.SCALAR],
argumentFct: (schema) => ({ args: [{ name: 'url', type: new NonNullType(schema.stringType()) }], errors: [] })
}),
// TODO: currently inconditionally adding @defer as the list of built-in. It's probably fine, but double check if we want to not do so when @defer-support is
// not enabled or something (it would probably be hard to handle it at that point anyway but well...).
// Note that @defer and @stream a inconditionally added to `Schema` even if they are technically "optional" built-in. _But_,
// the `Schema#toGraphQLJSSchema` method has an option to decide if @defer/@stream should be included or not in the resulting
// schema, which is how the gateway and router can, at runtime, decide to include or not include them based on actual support.
createDirectiveSpecification({
name: 'defer',
locations: [DirectiveLocation.FRAGMENT_SPREAD, DirectiveLocation.INLINE_FRAGMENT],
argumentFct: (schema) => ({
args: [
{ name: 'label', type: schema.stringType() },
{ name: 'if', type: schema.booleanType() },
{ name: 'if', type: new NonNullType(schema.booleanType()), defaultValue: true },
],
errors: [],
})
Expand All @@ -1151,17 +1152,14 @@ const graphQLBuiltInDirectivesSpecifications: readonly DirectiveSpecification[]
args: [
{ name: 'label', type: schema.stringType() },
{ name: 'initialCount', type: schema.intType(), defaultValue: 0 },
{ name: 'if', type: schema.booleanType() },
{ name: 'if', type: new NonNullType(schema.booleanType()), defaultValue: true },
],
errors: [],
})
}),
];

export type DeferDirectiveArgs = {
// TODO: we currently do not support variables for the defer label. Passing a label in a variable
// feels like a weird use case in the first place, but we should probably fix this nonetheless (or
// if we decide to have it be a known limitations, we should at least reject it cleanly).
label?: string,
if?: boolean | Variable,
}
Expand Down Expand Up @@ -1299,8 +1297,9 @@ export class Schema {
return nodes;
}

toGraphQLJSSchema(config?: { includeDefer?: boolean }): GraphQLSchema {
toGraphQLJSSchema(config?: { includeDefer?: boolean, includeStream?: boolean }): GraphQLSchema {
const includeDefer = config?.includeDefer ?? false;
const includeStream = config?.includeStream ?? false;

let ast = this.toAST();

Expand All @@ -1312,6 +1311,9 @@ export class Schema {
if (includeDefer) {
additionalNodes.push(this.deferDirective().toAST());
}
if (includeStream) {
additionalNodes.push(this.streamDirective().toAST());
}
if (additionalNodes.length > 0) {
ast = {
kind: Kind.DOCUMENT,
Expand Down
Loading

0 comments on commit 5204412

Please sign in to comment.