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 13 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: [],
isRepeatable: 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: [],
isRepeatable: 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>,
+isRepeatable: boolean,
Copy link
Contributor

Choose a reason for hiding this comment

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

I wonder if this should be:

+repeatable?: boolean,

If someone never uses repeatable directives, this adds no extra weight to the node.

I also don't think we need the is prefix: the fact that it's a boolean is enough information, and the question repeatable? false is close-to-valid English as-is (just as understandable as is repeatable? false).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The is prefix primarily added for consistency with isDeprecated, as @IvanGoncharov pointed out https://github.com/facebook/graphql/pull/472/files/1ba4e196aa710a10eceaa346d02a970dfd0b2d3a#diff-35fb422eded6e8f4df638c0d159008f6

I think I would prefer it keep it mandatory. This makes it easier to work with and i feel that the performance overhead is negligible. But I can also make it optional.

WDYT?

Copy link
Member

Choose a reason for hiding this comment

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

@OlegIlyenko In the context of AST, I'm more in favor of @mjmahone proposal since we already have block with similar prototype:

+block?: boolean,

Plus it looks like JS AST doesn't use is prefix, e.g. async:
https://github.com/estree/estree/blob/df2290b23a652e1ec354b6775459a3b42e22f108/es2017.md#function

But outside of AST I strongly believe it should be named isRepeatable for the consistency with isDeprecated.

This makes it easier to work with and i feel that the performance overhead is negligible.

I'm with you on that point especially since JS parser always preserve boolean flags and they typically work with way bigger files than GraphQL.
Also defacto (in our parser) we always provide values for all keys so why should Flow typings mark that as optional.

That said I think it's outside of scope of this PR and should be disscussed for AST as a whole and not for individual flags.
So for now I think it should be optional.

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 think both alternatives should be fine. I also considered keep repeatable name on AST nodes, but then renamed it with is prefix across the whole codebase for the sake of the naming consistency.

But I think it should totally fine to have repeatable field on AST node and then use isRepeatable everywhere else. @mjmahone What do you think?

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 renamed isRepeatable back to repeatable in all places except introspection: 89f5780

+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,
isRepeatable: 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, isRepeatable }) =>
'directive @' +
name +
(args.every(arg => arg.indexOf('\n') === -1)
? wrap('(', join(args, ', '), ')')
: wrap('(\n', indent(join(args, '\n')), '\n)')) +
(isRepeatable ? ' repeatable' : '') +
' on ' +
join(locations, ' | '),
),
Expand Down
90 changes: 90 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: 'isRepeatable',
type: {
kind: 'NON_NULL',
name: null,
ofType: {
kind: 'SCALAR',
name: 'Boolean',
ofType: null,
},
},
},
],
inputFields: null,
interfaces: [],
Expand Down Expand Up @@ -872,6 +887,81 @@ describe('Introspection', () => {
});
});

it('includes repeatable flag on directives', () => {
const EmptySchema = new GraphQLSchema({
query: new GraphQLObjectType({
name: 'QueryRoot',
fields: {
onlyField: { type: GraphQLString },
},
}),
});
const query = getIntrospectionQuery({
descriptions: false,
directiveRepeatableFlag: true,
});
const result = graphqlSync(EmptySchema, query);

expect((result.data: any).__schema.directives).to.deep.equal([
{
name: 'include',
locations: ['FIELD', 'FRAGMENT_SPREAD', 'INLINE_FRAGMENT'],
args: [
{
name: 'if',
type: {
kind: 'NON_NULL',
name: null,
ofType: {
kind: 'SCALAR',
name: 'Boolean',
ofType: null,
},
},
defaultValue: null,
},
],
isRepeatable: false,
},
{
name: 'skip',
locations: ['FIELD', 'FRAGMENT_SPREAD', 'INLINE_FRAGMENT'],
args: [
{
name: 'if',
type: {
kind: 'NON_NULL',
name: null,
ofType: {
kind: 'SCALAR',
name: 'Boolean',
ofType: null,
},
},
defaultValue: null,
},
],
isRepeatable: false,
},
{
name: 'deprecated',
locations: ['FIELD_DEFINITION', 'ENUM_VALUE'],
args: [
{
name: 'reason',
type: {
kind: 'SCALAR',
name: 'String',
ofType: null,
},
defaultValue: '"No longer supported"',
},
],
isRepeatable: false,
},
]);
});

it('introspects on input object', () => {
const TestInputObject = new GraphQLInputObjectType({
name: 'TestInputObject',
Expand Down
5 changes: 5 additions & 0 deletions src/type/directives.js
Expand Up @@ -44,12 +44,16 @@ export class GraphQLDirective {
locations: Array<DirectiveLocationEnum>;
args: Array<GraphQLArgument>;
astNode: ?DirectiveDefinitionNode;
isRepeatable: boolean;

constructor(config: GraphQLDirectiveConfig): void {
this.name = config.name;
this.description = config.description;
this.locations = config.locations;
this.astNode = config.astNode;
this.isRepeatable =
config.isRepeatable == null ? false : config.isRepeatable;

invariant(config.name, 'Directive must be named.');
invariant(
Array.isArray(config.locations),
Expand Down Expand Up @@ -92,6 +96,7 @@ export type GraphQLDirectiveConfig = {|
locations: Array<DirectiveLocationEnum>,
args?: ?GraphQLFieldConfigArgumentMap,
astNode?: ?DirectiveDefinitionNode,
isRepeatable?: ?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 || [],
},
isRepeatable: {
type: GraphQLNonNull(GraphQLBoolean),
description:
'Permits using the directive multiple times at the same location.',
resolve: directive => directive.isRepeatable,
},
}),
});

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').isRepeatable).to.equal(true);
});

it('Supports descriptions', () => {
const body = dedent`
"""This is a directive"""
Expand Down
20 changes: 18 additions & 2 deletions src/utilities/__tests__/buildClientSchema-test.js
Expand Up @@ -37,9 +37,19 @@ import {
// query against the client-side schema, it should get a result identical to
// what was returned by the server.
function testSchema(serverSchema) {
const initialIntrospection = introspectionFromSchema(serverSchema);
const introspectionOptions = {
descriptions: true,
directiveRepeatableFlag: true,
};
const initialIntrospection = introspectionFromSchema(
serverSchema,
introspectionOptions,
);
const clientSchema = buildClientSchema(initialIntrospection);
const secondIntrospection = introspectionFromSchema(clientSchema);
const secondIntrospection = introspectionFromSchema(
clientSchema,
introspectionOptions,
);
expect(secondIntrospection).to.deep.equal(initialIntrospection);
}

Expand Down Expand Up @@ -583,6 +593,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',
isRepeatable: true,
locations: ['FIELD'],
}),
],
});

Expand Down