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

Closed
wants to merge 15 commits into from
Closed
Show file tree
Hide file tree
Changes from 4 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
4 changes: 4 additions & 0 deletions src/language/__tests__/schema-kitchen-sink.graphql
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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:

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

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
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