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.

Sign up for GitHub

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] Added support for repeatable directives #1541

Closed
wants to merge 15 commits into from
Closed

[RFC] Added support for repeatable directives #1541

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
15 commits
Select commit Hold shift + click to select a range
cab37c7
[RFC] Added support for repeatable directives
OlegIlyenko Oct 2, 2018
ea40e38
Merge branch 'master' into repeatable-directives
OlegIlyenko Oct 2, 2018
7eb1fd7
Updated with the most recent changes
OlegIlyenko Oct 2, 2018
8cd0e52
Adjusted json formatting
OlegIlyenko Oct 2, 2018
77f85b3
Skip unknown directives + other improvements based on the review comm…
OlegIlyenko Oct 4, 2018
20deb26
Test for unknown directives
OlegIlyenko Oct 4, 2018
1180d92
Added support for repeatable directives in `extendSchema`
OlegIlyenko Oct 4, 2018
f8378d0
Added `DIRECTIVE_REPEATABLE_REMOVED` breaking change
OlegIlyenko Oct 4, 2018
3b5bf03
Skip `repeatable` flag by default in the introspection.
OlegIlyenko Oct 4, 2018
3a7fafa
`repeatable` flag in the introspection results is now optional
OlegIlyenko Oct 4, 2018
ae9e7f9
Use `uniqueDirectiveMap` instead of `repeatableMap`
OlegIlyenko Oct 4, 2018
56fee7d
Rename `repeatable` → `isRepeatable`
OlegIlyenko Oct 4, 2018
0dff21f
Updated `UniqueDirectivesPerLocation` validation docs
OlegIlyenko Oct 4, 2018
1a07bd9
Merge branch 'master' into repeatable-directives
OlegIlyenko Nov 6, 2018
89f5780
Use name `repeatable` instead of `isRepeatable` in all places except …
OlegIlyenko Nov 7, 2018
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
4 changes: 4 additions & 0 deletions src/language/__tests__/schema-kitchen-sink.graphql
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -124,6 +124,10 @@ directive @include2(if: Boolean!) on
| FRAGMENT_SPREAD
| INLINE_FRAGMENT

directive @myRepeatableDir(name: String!) repeatable on
| OBJECT
| INTERFACE

extend schema @onSchema

extend schema @onSchema {
Expand Down
102 changes: 102 additions & 0 deletions src/language/__tests__/schema-parser-test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -804,6 +804,108 @@ input Hello {
);
});

it('Directive definition', () => {
const body = 'directive @foo on OBJECT | INTERFACE';
const doc = parse(body);

expect(toJSONDeep(doc)).to.deep.equal({
kind: 'Document',
definitions: [
{
kind: 'DirectiveDefinition',
description: undefined,
name: {
kind: 'Name',
value: 'foo',
loc: {
start: 11,
end: 14,
},
},
arguments: [],
repeatable: false,
locations: [
{
kind: 'Name',
value: 'OBJECT',
loc: {
start: 18,
end: 24,
},
},
{
kind: 'Name',
value: 'INTERFACE',
loc: {
start: 27,
end: 36,
},
},
],
loc: {
start: 0,
end: 36,
},
},
],
loc: {
start: 0,
end: 36,
},
});
});

it('Repeatable directive definition', () => {
const body = 'directive @foo repeatable on OBJECT | INTERFACE';
const doc = parse(body);

expect(toJSONDeep(doc)).to.deep.equal({
kind: 'Document',
definitions: [
{
kind: 'DirectiveDefinition',
description: undefined,
name: {
kind: 'Name',
value: 'foo',
loc: {
start: 11,
end: 14,
},
},
arguments: [],
repeatable: true,
locations: [
{
kind: 'Name',
value: 'OBJECT',
loc: {
start: 29,
end: 35,
},
},
{
kind: 'Name',
value: 'INTERFACE',
loc: {
start: 38,
end: 47,
},
},
],
loc: {
start: 0,
end: 47,
},
},
],
loc: {
start: 0,
end: 47,
},
});
});

it('Directive with incorrect locations', () => {
expectSyntaxError(
`
Expand Down
2 changes: 2 additions & 0 deletions src/language/__tests__/schema-printer-test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -164,6 +164,8 @@ describe('Printer: SDL document', () => {

directive @include2(if: Boolean!) on FIELD | FRAGMENT_SPREAD | INLINE_FRAGMENT

directive @myRepeatableDir(name: String!) repeatable on OBJECT | INTERFACE

extend schema @onSchema

extend schema @onSchema {
Expand Down
1 change: 1 addition & 0 deletions src/language/ast.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -508,6 +508,7 @@ export type DirectiveDefinitionNode = {
+description?: StringValueNode,
+name: NameNode,
+arguments?: $ReadOnlyArray<InputValueDefinitionNode>,
+repeatable: boolean,
+locations: $ReadOnlyArray<NameNode>,
};

Expand Down
19 changes: 18 additions & 1 deletion src/language/parser.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1378,7 +1378,7 @@ function parseInputObjectTypeExtension(

/**
* DirectiveDefinition :
* - Description? directive @ Name ArgumentsDefinition? on DirectiveLocations
* - Description? directive @ Name ArgumentsDefinition? `repeatable`? on DirectiveLocations
*/
function parseDirectiveDefinition(lexer: Lexer<*>): DirectiveDefinitionNode {
const start = lexer.token;
Expand All @@ -1387,13 +1387,15 @@ function parseDirectiveDefinition(lexer: Lexer<*>): DirectiveDefinitionNode {
expect(lexer, TokenKind.AT);
const name = parseName(lexer);
const args = parseArgumentDefs(lexer);
const repeatable = expectOptionalKeyword(lexer, 'repeatable');
expectKeyword(lexer, 'on');
const locations = parseDirectiveLocations(lexer);
return {
kind: Kind.DIRECTIVE_DEFINITION,
description,
name,
arguments: args,
repeatable,
locations,
loc: loc(lexer, start),
};
Expand Down Expand Up @@ -1529,6 +1531,21 @@ function expectKeyword(lexer: Lexer<*>, value: string): Token {
);
}

/**
* If the next token is a keyword with the given value, return true after advancing
* the lexer. Otherwise, do not change the parser state and return false.
*/
function expectOptionalKeyword(lexer: Lexer<*>, value: string): boolean {
const token = lexer.token;
const match = token.kind === TokenKind.NAME && token.value === value;

if (match) {
lexer.advance();
}

return match;
}

/**
* Helper function for creating an error when an unexpected lexed token
* is encountered.
Expand Down
3 changes: 2 additions & 1 deletion src/language/printer.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -181,12 +181,13 @@ const printDocASTReducer = {
),

DirectiveDefinition: addDescription(
({ name, arguments: args, locations }) =>
({ name, arguments: args, locations, repeatable }) =>
'directive @' +
name +
(args.every(arg => arg.indexOf('\n') === -1)
? wrap('(', join(args, ', '), ')')
: wrap('(\n', indent(join(args, '\n')), '\n)')) +
(repeatable ? ' repeatable' : '') +
' on ' +
join(locations, ' | '),
),
Expand Down
18 changes: 18 additions & 0 deletions src/type/__tests__/introspection-test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -700,6 +700,21 @@ describe('Introspection', () => {
isDeprecated: false,
deprecationReason: null,
},
{
args: [],
deprecationReason: null,
isDeprecated: false,
name: 'repeatable',
type: {
kind: 'NON_NULL',
name: null,
ofType: {
kind: 'SCALAR',
name: 'Boolean',
ofType: null,
},
},
},
],
inputFields: null,
interfaces: [],
Expand Down Expand Up @@ -815,6 +830,7 @@ describe('Introspection', () => {
directives: [
{
name: 'include',
repeatable: false,
locations: ['FIELD', 'FRAGMENT_SPREAD', 'INLINE_FRAGMENT'],
args: [
{
Expand All @@ -834,6 +850,7 @@ describe('Introspection', () => {
},
{
name: 'skip',
repeatable: false,
locations: ['FIELD', 'FRAGMENT_SPREAD', 'INLINE_FRAGMENT'],
args: [
{
Expand All @@ -853,6 +870,7 @@ describe('Introspection', () => {
},
{
name: 'deprecated',
repeatable: false,
locations: ['FIELD_DEFINITION', 'ENUM_VALUE'],
args: [
{
Expand Down
6 changes: 6 additions & 0 deletions src/type/directives.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,12 +44,17 @@ export class GraphQLDirective {
locations: Array<DirectiveLocationEnum>;
args: Array<GraphQLArgument>;
astNode: ?DirectiveDefinitionNode;
repeatable: boolean;

constructor(config: GraphQLDirectiveConfig): void {
this.name = config.name;
this.description = config.description;
this.locations = config.locations;
this.astNode = config.astNode;
this.repeatable =
config.repeatable === undefined || config.repeatable === null
Copy link
Member

Choose a reason for hiding this comment

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

In JS you can do that with a single config.repeatable == null and we already using it in couple places.

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 believe config.repeatable === undefined is still necessary since I defined the flow type like this:

repeatable?: ?boolean,

If I don't do the check, I will get following flow error:

Cannot assign `config.repeatable === null ? false : config.repeatable` to `this.repeatable` because:
 - null or undefined [1] is incompatible with boolean [2].
 - undefined [1] is incompatible with boolean [2].

Copy link
Member

Choose a reason for hiding this comment

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

@OlegIlyenko Note == null (two equal signs) compares both to null and undefined and Flow should handle it without problems.

If I don't do the check, I will get following flow error:

It's because you use === instead of ==.
It's safe and very common pattern in JS to do == null:
https://eslint.org/docs/rules/eqeqeq#smart

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah, I see. Haven't noticed this, thanks for the clarification.

I got an impression that the use of == is discouraged in javascript community since it relies of a set of coercion rules that many people find error-prone. This is why I tried to avoid using it. I was not aware of the exceptions described in the eslint documentation though. I will update the code and use ==.

? false
: config.repeatable;
invariant(config.name, 'Directive must be named.');
invariant(
Array.isArray(config.locations),
Expand Down Expand Up @@ -92,6 +97,7 @@ export type GraphQLDirectiveConfig = {|
locations: Array<DirectiveLocationEnum>,
args?: ?GraphQLFieldConfigArgumentMap,
astNode?: ?DirectiveDefinitionNode,
repeatable?: ?boolean,
|};

/**
Expand Down
6 changes: 6 additions & 0 deletions src/type/introspection.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -98,6 +98,12 @@ export const __Directive = new GraphQLObjectType({
type: GraphQLNonNull(GraphQLList(GraphQLNonNull(__InputValue))),
resolve: directive => directive.args || [],
},
repeatable: {
type: GraphQLNonNull(GraphQLBoolean),
description:
'Permits using the directive multiple times at the same location.',
resolve: directive => directive.repeatable,
},
}),
});

Expand Down
16 changes: 16 additions & 0 deletions src/utilities/__tests__/buildASTSchema-test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -91,6 +91,22 @@ describe('Schema Builder', () => {
expect(output).to.equal(body);
});

it('With repeatable directives', () => {
const body = dedent`
directive @foo(arg: Int) repeatable on FIELD

type Query {
str: String
}
`;

const output = cycleOutput(body);
expect(output).to.equal(body);

const schema = buildASTSchema(parse(body));
expect(schema.getDirective('foo').repeatable).to.equal(true);
});

it('Supports descriptions', () => {
const body = dedent`
"""This is a directive"""
Expand Down
6 changes: 6 additions & 0 deletions src/utilities/__tests__/buildClientSchema-test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -583,6 +583,12 @@ describe('Type System: build schema from introspection', () => {
description: 'This is a custom directive',
locations: ['FIELD'],
}),
new GraphQLDirective({
name: 'customRepeatableDirective',
description: 'This is a custom repeatable directive',
repeatable: true,
locations: ['FIELD'],
}),
],
});

Expand Down
6 changes: 6 additions & 0 deletions src/utilities/__tests__/schemaPrinter-test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -650,6 +650,9 @@ describe('Type System Printer', () => {
description: String
locations: [__DirectiveLocation!]!
args: [__InputValue!]!

"""Permits using the directive multiple times at the same location."""
repeatable: Boolean!
}

"""
Expand Down Expand Up @@ -883,6 +886,9 @@ describe('Type System Printer', () => {
description: String
locations: [__DirectiveLocation!]!
args: [__InputValue!]!

# Permits using the directive multiple times at the same location.
repeatable: Boolean!
}

# A Directive can be adjacent to many parts of the GraphQL language, a
Expand Down
1 change: 1 addition & 0 deletions src/utilities/buildASTSchema.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -282,6 +282,7 @@ export class ASTDefinitionBuilder {
args:
directiveNode.arguments &&
this._makeInputValues(directiveNode.arguments),
repeatable: directiveNode.repeatable,
astNode: directiveNode,
});
}
Expand Down
1 change: 1 addition & 0 deletions src/utilities/buildClientSchema.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -355,6 +355,7 @@ export function buildClientSchema(
description: directiveIntrospection.description,
locations: directiveIntrospection.locations.slice(),
args: buildInputValueDefMap(directiveIntrospection.args),
repeatable: directiveIntrospection.repeatable,
});
}

Expand Down
2 changes: 2 additions & 0 deletions src/utilities/introspectionQuery.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,7 @@ export function getIntrospectionQuery(options?: IntrospectionOptions): string {
args {
...InputValue
}
repeatable
Copy link
Member

Choose a reason for hiding this comment

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

@OlegIlyenko As discussed on the working group we can't assume that legacy servers would support repeatable. So this change will break GraphiQL and many other tools.

I would suggest making option directiveRepeatable similar to this one:

graphql-js/src/utilities/introspectionQuery.js

Lines 12 to 16 in f8378d0

export type IntrospectionOptions = {|
// Whether to include descriptions in the introspection result.
// Default: true
descriptions: boolean,
|};

Long term solution would be to implement two-stage introspection as suggested by Lee.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's a good point, I added an option for it 3b5bf03 I also made if disabled by defult in order to maintain backwards compatibility.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Also this one: 3a7fafa I forgot to update the introspection flow type

}
}
}
Expand Down Expand Up @@ -273,4 +274,5 @@ export type IntrospectionDirective = {|
+description?: ?string,
+locations: $ReadOnlyArray<DirectiveLocationEnum>,
+args: $ReadOnlyArray<IntrospectionInputValue>,
+repeatable: boolean,
Copy link
Member

Choose a reason for hiding this comment

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

@OlegIlyenko Can you please address this comment in spec RFC:
https://github.com/facebook/graphql/pull/472/files/1ba4e196aa710a10eceaa346d02a970dfd0b2d3a#diff-35fb422eded6e8f4df638c0d159008f6

It blocks this PR but I think we should discuss it in the scope of spec RFC.

Copy link
Contributor Author

@OlegIlyenko OlegIlyenko Oct 4, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

Could you please describe in more detail what kind of information you would like to add? I'm not 100% sure I understood your comment.

Copy link
Member

Choose a reason for hiding this comment

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

@OlegIlyenko I mean if merge this PR that will expose repeatable through introspection and it would be significantly harder to rename it later.
As I wrote in my comment to the spec RFC I think this field should be named isRepeatable for consistency with isDeprecated.

Everything else in this PR looks good so would be great to discuss this last point but since this PR is just an implementation of spec RFC I think we should discuss it in spec PR.

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 see, thanks for the clarification. Sure thing, I will open a discussion on this topic in the RFC PR.

Copy link
Member

Choose a reason for hiding this comment

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

@OlegIlyenko I feel very stupid because in the first comment of this thread I linked review comment I forget to submit (review comments UI is very weird).
I already submitted it in.

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 see, thanks! now it's clear, i was confused about wrong link

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 did the renaming 56fee7d I will adjust the spec text accordingly

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh I just saw this comment. Sorry for the late comment: I guess keeping consistent with isDeprecated makes sense, though it still feels a little weird. Oh well.

|};
1 change: 1 addition & 0 deletions src/utilities/schemaPrinter.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -309,6 +309,7 @@ function printDirective(directive, options) {
'directive @' +
directive.name +
printArgs(options, directive.args) +
(directive.repeatable ? ' repeatable' : '') +
' on ' +
directive.locations.join(' | ')
);
Expand Down
Loading