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

[RFC] Allow directives on variable definitions #1437

Merged
merged 4 commits into from
Aug 7, 2018
Merged
Show file tree
Hide file tree
Changes from all 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
6 changes: 6 additions & 0 deletions src/language/__tests__/parser-test.js
Original file line number Diff line number Diff line change
Expand Up @@ -106,6 +106,12 @@ describe('Parser', () => {
);
});

it('parses variable definition directives', () => {
expect(() =>
parse('query Foo($x: Boolean = false @bar) { field }'),
).to.not.throw();
});

it('does not accept fragments named "on"', () => {
expectSyntaxError('fragment on on on { on }', 'Unexpected Name "on"', {
line: 1,
Expand Down
9 changes: 9 additions & 0 deletions src/language/__tests__/printer-test.js
Original file line number Diff line number Diff line change
Expand Up @@ -64,6 +64,15 @@ describe('Printer: Query document', () => {
}
`);

const queryAstWithVariableDirective = parse(
'query ($foo: TestType = {a: 123} @testDirective(if: true) @test) { id }',
);
expect(print(queryAstWithVariableDirective)).to.equal(dedent`
query ($foo: TestType = {a: 123} @testDirective(if: true) @test) {
id
}
`);

const mutationAstWithArtifacts = parse(
'mutation ($foo: TestType) @testDirective { id, name }',
);
Expand Down
1 change: 1 addition & 0 deletions src/language/ast.js
Original file line number Diff line number Diff line change
Expand Up @@ -225,6 +225,7 @@ export type VariableDefinitionNode = {
+variable: VariableNode,
+type: TypeNode,
+defaultValue?: ValueNode,
+directives?: $ReadOnlyArray<DirectiveNode>,
};

export type VariableNode = {
Expand Down
1 change: 1 addition & 0 deletions src/language/directiveLocation.js
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@ export const DirectiveLocation = Object.freeze({
FRAGMENT_DEFINITION: 'FRAGMENT_DEFINITION',
FRAGMENT_SPREAD: 'FRAGMENT_SPREAD',
INLINE_FRAGMENT: 'INLINE_FRAGMENT',
VARIABLE_DEFINITION: 'VARIABLE_DEFINITION',
// Type System Definitions
SCHEMA: 'SCHEMA',
SCALAR: 'SCALAR',
Expand Down
3 changes: 2 additions & 1 deletion src/language/parser.js
Original file line number Diff line number Diff line change
Expand Up @@ -332,7 +332,7 @@ function parseVariableDefinitions(
}

/**
* VariableDefinition : Variable : Type DefaultValue?
* VariableDefinition : Variable : Type DefaultValue? Directives[Const]?
*/
function parseVariableDefinition(lexer: Lexer<*>): VariableDefinitionNode {
const start = lexer.token;
Expand All @@ -343,6 +343,7 @@ function parseVariableDefinition(lexer: Lexer<*>): VariableDefinitionNode {
defaultValue: skip(lexer, TokenKind.EQUALS)
? parseValueLiteral(lexer, true)
: undefined,
directives: parseDirectives(lexer, true),
loc: loc(lexer, start),
};
}
Expand Down
9 changes: 6 additions & 3 deletions src/language/printer.js
Original file line number Diff line number Diff line change
Expand Up @@ -36,9 +36,12 @@ const printDocASTReducer = {
: join([op, join([name, varDefs]), directives, selectionSet], ' ');
},

VariableDefinition: ({ variable, type, defaultValue }) =>
variable + ': ' + type + wrap(' = ', defaultValue),

VariableDefinition: ({ variable, type, defaultValue, directives }) =>
variable +
': ' +
type +
wrap(' = ', defaultValue) +
wrap(' ', join(directives, ' ')),
SelectionSet: ({ selections }) => block(selections),

Field: ({ alias, name, arguments: args, directives, selectionSet }) =>
Expand Down
2 changes: 1 addition & 1 deletion src/language/visitor.js
Original file line number Diff line number Diff line change
Expand Up @@ -64,7 +64,7 @@ export const QueryDocumentKeys = {
'directives',
'selectionSet',
],
VariableDefinition: ['variable', 'type', 'defaultValue'],
VariableDefinition: ['variable', 'type', 'defaultValue', 'directives'],
Copy link
Contributor

Choose a reason for hiding this comment

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

These typically follow the order found in the grammar, but this seems inconsistent

Copy link
Contributor

Choose a reason for hiding this comment

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

My mistake. Consistent for this PR but inconsistent with the spec RFC

Variable: ['name'],
SelectionSet: ['selections'],
Field: ['alias', 'name', 'arguments', 'directives', 'selectionSet'],
Expand Down
5 changes: 5 additions & 0 deletions src/type/__tests__/introspection-test.js
Original file line number Diff line number Diff line change
Expand Up @@ -746,6 +746,11 @@ describe('Introspection', () => {
isDeprecated: false,
deprecationReason: null,
},
{
name: 'VARIABLE_DEFINITION',
isDeprecated: false,
deprecationReason: null,
},
{
name: 'SCHEMA',
isDeprecated: false,
Expand Down
4 changes: 4 additions & 0 deletions src/type/introspection.js
Original file line number Diff line number Diff line change
Expand Up @@ -135,6 +135,10 @@ export const __DirectiveLocation = new GraphQLEnumType({
value: DirectiveLocation.INLINE_FRAGMENT,
description: 'Location adjacent to an inline fragment.',
},
VARIABLE_DEFINITION: {
value: DirectiveLocation.VARIABLE_DEFINITION,
description: 'Location adjacent to a variable definition.',
},
SCHEMA: {
value: DirectiveLocation.SCHEMA,
description: 'Location adjacent to a schema definition.',
Expand Down
6 changes: 6 additions & 0 deletions src/utilities/__tests__/schemaPrinter-test.js
Original file line number Diff line number Diff line change
Expand Up @@ -676,6 +676,9 @@ describe('Type System Printer', () => {
"""Location adjacent to an inline fragment."""
INLINE_FRAGMENT

"""Location adjacent to a variable definition."""
VARIABLE_DEFINITION

"""Location adjacent to a schema definition."""
SCHEMA

Expand Down Expand Up @@ -904,6 +907,9 @@ describe('Type System Printer', () => {
# Location adjacent to an inline fragment.
INLINE_FRAGMENT

# Location adjacent to a variable definition.
VARIABLE_DEFINITION

# Location adjacent to a schema definition.
SCHEMA

Expand Down
11 changes: 6 additions & 5 deletions src/validation/__tests__/KnownDirectives-test.js
Original file line number Diff line number Diff line change
Expand Up @@ -127,8 +127,8 @@ describe('Validate: Known directives', () => {
expectPassesRule(
KnownDirectives,
`
query Foo @onQuery {
name @include(if: true)
query Foo($var: Boolean @onVariableDefinition) @onQuery {
name @include(if: $var)
...Frag @include(if: true)
skippedField @skip(if: true)
...SkippedFrag @skip(if: true)
Expand All @@ -145,8 +145,8 @@ describe('Validate: Known directives', () => {
expectFailsRule(
KnownDirectives,
`
query Foo @include(if: true) {
name @onQuery
query Foo($var: Boolean @onField) @include(if: true) {
name @onQuery @include(if: $var)
...Frag @onQuery
}

Expand All @@ -155,7 +155,8 @@ describe('Validate: Known directives', () => {
}
`,
[
misplacedDirective('include', 'QUERY', 2, 17),
misplacedDirective('onField', 'VARIABLE_DEFINITION', 2, 31),
misplacedDirective('include', 'QUERY', 2, 41),
misplacedDirective('onQuery', 'FIELD', 3, 14),
misplacedDirective('onQuery', 'FRAGMENT_SPREAD', 4, 17),
misplacedDirective('onQuery', 'MUTATION', 7, 20),
Expand Down
4 changes: 4 additions & 0 deletions src/validation/__tests__/harness.js
Original file line number Diff line number Diff line change
Expand Up @@ -374,6 +374,10 @@ export const testSchema = new GraphQLSchema({
name: 'onInlineFragment',
locations: ['INLINE_FRAGMENT'],
}),
new GraphQLDirective({
name: 'onVariableDefinition',
locations: ['VARIABLE_DEFINITION'],
}),
],
});

Expand Down
2 changes: 2 additions & 0 deletions src/validation/rules/KnownDirectives.js
Original file line number Diff line number Diff line change
Expand Up @@ -98,6 +98,8 @@ function getDirectiveLocationForASTPath(ancestors) {
return DirectiveLocation.INLINE_FRAGMENT;
case Kind.FRAGMENT_DEFINITION:
return DirectiveLocation.FRAGMENT_DEFINITION;
case Kind.VARIABLE_DEFINITION:
return DirectiveLocation.VARIABLE_DEFINITION;
case Kind.SCHEMA_DEFINITION:
case Kind.SCHEMA_EXTENSION:
return DirectiveLocation.SCHEMA;
Expand Down