From 2033b21a41120c0dcd2d901873ec06509575b046 Mon Sep 17 00:00:00 2001 From: Matt Mahoney Date: Thu, 9 Feb 2023 16:09:43 -0500 Subject: [PATCH] Fragment Arguments: Only Syntax Changes --- src/language/__tests__/parser-test.ts | 25 +++++++-- src/language/__tests__/printer-test.ts | 44 +++++++++++++--- src/language/__tests__/visitor-test.ts | 48 ++++++++++++++++- src/language/ast.ts | 5 +- src/language/directiveLocation.ts | 1 + src/language/parser.ts | 52 +++++++++++-------- src/language/printer.ts | 27 +++++++--- src/type/__tests__/introspection-test.ts | 5 ++ src/type/introspection.ts | 6 ++- src/utilities/__tests__/printSchema-test.ts | 5 +- src/utilities/buildASTSchema.ts | 2 +- .../__tests__/KnownDirectivesRule-test.ts | 14 +++-- src/validation/__tests__/harness.ts | 2 +- src/validation/rules/KnownDirectivesRule.ts | 10 +++- 14 files changed, 190 insertions(+), 56 deletions(-) diff --git a/src/language/__tests__/parser-test.ts b/src/language/__tests__/parser-test.ts index 34e9dff4b9..215e743be7 100644 --- a/src/language/__tests__/parser-test.ts +++ b/src/language/__tests__/parser-test.ts @@ -607,13 +607,32 @@ describe('Parser', () => { expect('loc' in result).to.equal(false); }); - it('Legacy: allows parsing fragment defined variables', () => { + it('allows parsing fragment defined variables', () => { const document = 'fragment a($v: Boolean = false) on t { f(v: $v) }'; expect(() => - parse(document, { allowLegacyFragmentVariables: true }), + parse(document, { experimentalFragmentArguments: true }), ).to.not.throw(); - expect(() => parse(document)).to.throw('Syntax Error'); + }); + + it('disallows parsing fragment defined variables without experimental flag', () => { + const document = 'fragment a($v: Boolean = false) on t { f(v: $v) }'; + + expect(() => parse(document)).to.throw(); + }); + + it('allows parsing fragment spread arguments', () => { + const document = 'fragment a on t { ...b(v: $v) }'; + + expect(() => + parse(document, { experimentalFragmentArguments: true }), + ).to.not.throw(); + }); + + it('disallows parsing fragment spread arguments without experimental flag', () => { + const document = 'fragment a on t { ...b(v: $v) }'; + + expect(() => parse(document)).to.throw(); }); it('contains location that can be Object.toStringified, JSON.stringified, or jsutils.inspected', () => { diff --git a/src/language/__tests__/printer-test.ts b/src/language/__tests__/printer-test.ts index 0585cae6d9..76375cc7a3 100644 --- a/src/language/__tests__/printer-test.ts +++ b/src/language/__tests__/printer-test.ts @@ -110,34 +110,62 @@ describe('Printer: Query document', () => { `); }); - it('Legacy: prints fragment with variable directives', () => { - const queryASTWithVariableDirective = parse( + it('prints fragment with argument definition directives', () => { + const fragmentWithArgumentDefinitionDirective = parse( 'fragment Foo($foo: TestType @test) on TestType @testDirective { id }', - { allowLegacyFragmentVariables: true }, + { experimentalFragmentArguments: true }, ); - expect(print(queryASTWithVariableDirective)).to.equal(dedent` + expect(print(fragmentWithArgumentDefinitionDirective)).to.equal(dedent` fragment Foo($foo: TestType @test) on TestType @testDirective { id } `); }); - it('Legacy: correctly prints fragment defined variables', () => { - const fragmentWithVariable = parse( + it('correctly prints fragment defined arguments', () => { + const fragmentWithArgumentDefinition = parse( ` fragment Foo($a: ComplexType, $b: Boolean = false) on TestType { id } `, - { allowLegacyFragmentVariables: true }, + { experimentalFragmentArguments: true }, ); - expect(print(fragmentWithVariable)).to.equal(dedent` + expect(print(fragmentWithArgumentDefinition)).to.equal(dedent` fragment Foo($a: ComplexType, $b: Boolean = false) on TestType { id } `); }); + it('prints fragment spread with arguments', () => { + const fragmentSpreadWithArguments = parse( + 'fragment Foo on TestType { ...Bar(a: {x: $x}, b: true) }', + { experimentalFragmentArguments: true }, + ); + expect(print(fragmentSpreadWithArguments)).to.equal(dedent` + fragment Foo on TestType { + ...Bar(a: { x: $x }, b: true) + } + `); + }); + + it('prints fragment spread with multi-line arguments', () => { + const fragmentSpreadWithArguments = parse( + 'fragment Foo on TestType { ...Bar(a: {x: $x, y: $y, z: $z, xy: $xy}, b: true, c: "a long string extending arguments over max length") }', + { experimentalFragmentArguments: true }, + ); + expect(print(fragmentSpreadWithArguments)).to.equal(dedent` + fragment Foo on TestType { + ...Bar( + a: { x: $x, y: $y, z: $z, xy: $xy } + b: true + c: "a long string extending arguments over max length" + ) + } + `); + }); + it('prints kitchen sink without altering ast', () => { const ast = parse(kitchenSinkQuery, { noLocation: true, diff --git a/src/language/__tests__/visitor-test.ts b/src/language/__tests__/visitor-test.ts index 270c948225..0af34fdadd 100644 --- a/src/language/__tests__/visitor-test.ts +++ b/src/language/__tests__/visitor-test.ts @@ -455,10 +455,10 @@ describe('Visitor', () => { ]); }); - it('Legacy: visits variables defined in fragments', () => { + it('visits arguments defined on fragments', () => { const ast = parse('fragment a($v: Boolean = false) on t { f }', { noLocation: true, - allowLegacyFragmentVariables: true, + experimentalFragmentArguments: true, }); const visited: Array = []; @@ -505,6 +505,50 @@ describe('Visitor', () => { ]); }); + it('visits arguments on fragment spreads', () => { + const ast = parse('fragment a on t { ...s(v: false) }', { + noLocation: true, + experimentalFragmentArguments: true, + }); + const visited: Array = []; + + visit(ast, { + enter(node) { + checkVisitorFnArgs(ast, arguments); + visited.push(['enter', node.kind, getValue(node)]); + }, + leave(node) { + checkVisitorFnArgs(ast, arguments); + visited.push(['leave', node.kind, getValue(node)]); + }, + }); + + expect(visited).to.deep.equal([ + ['enter', 'Document', undefined], + ['enter', 'FragmentDefinition', undefined], + ['enter', 'Name', 'a'], + ['leave', 'Name', 'a'], + ['enter', 'NamedType', undefined], + ['enter', 'Name', 't'], + ['leave', 'Name', 't'], + ['leave', 'NamedType', undefined], + ['enter', 'SelectionSet', undefined], + ['enter', 'FragmentSpread', undefined], + ['enter', 'Name', 's'], + ['leave', 'Name', 's'], + ['enter', 'Argument', { kind: 'BooleanValue', value: false }], + ['enter', 'Name', 'v'], + ['leave', 'Name', 'v'], + ['enter', 'BooleanValue', false], + ['leave', 'BooleanValue', false], + ['leave', 'Argument', { kind: 'BooleanValue', value: false }], + ['leave', 'FragmentSpread', undefined], + ['leave', 'SelectionSet', undefined], + ['leave', 'FragmentDefinition', undefined], + ['leave', 'Document', undefined], + ]); + }); + it('n', () => { const ast = parse(kitchenSinkQuery, { experimentalClientControlledNullability: true, diff --git a/src/language/ast.ts b/src/language/ast.ts index 22a7cc253c..1f387056e8 100644 --- a/src/language/ast.ts +++ b/src/language/ast.ts @@ -227,11 +227,10 @@ export const QueryDocumentKeys: { NonNullAssertion: ['nullabilityAssertion'], ErrorBoundary: ['nullabilityAssertion'], - FragmentSpread: ['name', 'directives'], + FragmentSpread: ['name', 'arguments', 'directives'], InlineFragment: ['typeCondition', 'directives', 'selectionSet'], FragmentDefinition: [ 'name', - // Note: fragment variable definitions are deprecated and will removed in v17.0.0 'variableDefinitions', 'typeCondition', 'directives', @@ -428,6 +427,7 @@ export interface FragmentSpreadNode { readonly kind: Kind.FRAGMENT_SPREAD; readonly loc?: Location | undefined; readonly name: NameNode; + readonly arguments?: ReadonlyArray | undefined; readonly directives?: ReadonlyArray | undefined; } @@ -443,7 +443,6 @@ export interface FragmentDefinitionNode { readonly kind: Kind.FRAGMENT_DEFINITION; readonly loc?: Location | undefined; readonly name: NameNode; - /** @deprecated variableDefinitions will be removed in v17.0.0 */ readonly variableDefinitions?: | ReadonlyArray | undefined; diff --git a/src/language/directiveLocation.ts b/src/language/directiveLocation.ts index b10214d47f..9732b2c487 100644 --- a/src/language/directiveLocation.ts +++ b/src/language/directiveLocation.ts @@ -11,6 +11,7 @@ export enum DirectiveLocation { FRAGMENT_SPREAD = 'FRAGMENT_SPREAD', INLINE_FRAGMENT = 'INLINE_FRAGMENT', VARIABLE_DEFINITION = 'VARIABLE_DEFINITION', + FRAGMENT_VARIABLE_DEFINITION = 'FRAGMENT_VARIABLE_DEFINITION', /** Type System Definitions */ SCHEMA = 'SCHEMA', SCALAR = 'SCALAR', diff --git a/src/language/parser.ts b/src/language/parser.ts index bc58875e9d..b8b1408239 100644 --- a/src/language/parser.ts +++ b/src/language/parser.ts @@ -92,21 +92,25 @@ export interface ParseOptions { maxTokens?: number | undefined; /** - * @deprecated will be removed in the v17.0.0 + * EXPERIMENTAL: * - * If enabled, the parser will understand and parse variable definitions - * contained in a fragment definition. They'll be represented in the - * `variableDefinitions` field of the FragmentDefinitionNode. + * If enabled, the parser will understand and parse fragment variable definitions + * and arguments on fragment spreads. Fragment variable definitions will be represented + * in the `variableDefinitions` field of the FragmentDefinitionNode. + * Fragment spread arguments will be represented in the `arguments` field of FragmentSpreadNode. * - * The syntax is identical to normal, query-defined variables. For example: + * For example: * * ```graphql + * { + * t { ...A(var: true) } + * } * fragment A($var: Boolean = false) on T { - * ... + * ...B(x: $var) * } * ``` */ - allowLegacyFragmentVariables?: boolean | undefined; + experimentalFragmentArguments?: boolean | undefined; /** * EXPERIMENTAL: @@ -550,7 +554,7 @@ export class Parser { /** * Corresponds to both FragmentSpread and InlineFragment in the spec. * - * FragmentSpread : ... FragmentName Directives? + * FragmentSpread : ... FragmentName Arguments? Directives? * * InlineFragment : ... TypeCondition? Directives? SelectionSet */ @@ -560,9 +564,21 @@ export class Parser { const hasTypeCondition = this.expectOptionalKeyword('on'); if (!hasTypeCondition && this.peek(TokenKind.NAME)) { + const name = this.parseFragmentName(); + if ( + this.peek(TokenKind.PAREN_L) && + this._options.experimentalFragmentArguments + ) { + return this.node(start, { + kind: Kind.FRAGMENT_SPREAD, + name, + arguments: this.parseArguments(false), + directives: this.parseDirectives(false), + }); + } return this.node(start, { kind: Kind.FRAGMENT_SPREAD, - name: this.parseFragmentName(), + name, directives: this.parseDirectives(false), }); } @@ -576,29 +592,19 @@ export class Parser { /** * FragmentDefinition : - * - fragment FragmentName on TypeCondition Directives? SelectionSet + * - fragment FragmentName VariableDefinitions? on TypeCondition Directives? SelectionSet * * TypeCondition : NamedType */ parseFragmentDefinition(): FragmentDefinitionNode { const start = this._lexer.token; this.expectKeyword('fragment'); - // Legacy support for defining variables within fragments changes - // the grammar of FragmentDefinition: - // - fragment FragmentName VariableDefinitions? on TypeCondition Directives? SelectionSet - if (this._options.allowLegacyFragmentVariables === true) { - return this.node(start, { - kind: Kind.FRAGMENT_DEFINITION, - name: this.parseFragmentName(), - variableDefinitions: this.parseVariableDefinitions(), - typeCondition: (this.expectKeyword('on'), this.parseNamedType()), - directives: this.parseDirectives(false), - selectionSet: this.parseSelectionSet(), - }); - } return this.node(start, { kind: Kind.FRAGMENT_DEFINITION, name: this.parseFragmentName(), + variableDefinitions: this._options.experimentalFragmentArguments + ? this.parseVariableDefinitions() + : undefined, typeCondition: (this.expectKeyword('on'), this.parseNamedType()), directives: this.parseDirectives(false), selectionSet: this.parseSelectionSet(), diff --git a/src/language/printer.ts b/src/language/printer.ts index a07decc11d..cd1c187eb3 100644 --- a/src/language/printer.ts +++ b/src/language/printer.ts @@ -64,14 +64,9 @@ const printDocASTReducer: ASTReducer = { selectionSet, }) { const prefix = join([wrap('', alias, ': '), name], ''); - let argsLine = prefix + wrap('(', join(args, ', '), ')'); - - if (argsLine.length > MAX_LINE_LENGTH) { - argsLine = prefix + wrap('(\n', indent(join(args, '\n')), '\n)'); - } return join([ - argsLine, + wrappedLineAndArgs(prefix, args), // Note: Client Controlled Nullability is experimental and may be // changed or removed in the future. nullabilityAssertion, @@ -105,8 +100,12 @@ const printDocASTReducer: ASTReducer = { // Fragments FragmentSpread: { - leave: ({ name, directives }) => - '...' + name + wrap(' ', join(directives, ' ')), + leave: ({ name, arguments: args, directives }) => { + const prefix = '...' + name; + return ( + wrappedLineAndArgs(prefix, args) + wrap(' ', join(directives, ' ')) + ); + }, }, InlineFragment: { @@ -378,3 +377,15 @@ function hasMultilineItems(maybeArray: Maybe>): boolean { /* c8 ignore next */ return maybeArray?.some((str) => str.includes('\n')) ?? false; } + +function wrappedLineAndArgs( + prefix: string, + args: ReadonlyArray | undefined, +): string { + let argsLine = prefix + wrap('(', join(args, ', '), ')'); + + if (argsLine.length > MAX_LINE_LENGTH) { + argsLine = prefix + wrap('(\n', indent(join(args, '\n')), '\n)'); + } + return argsLine; +} diff --git a/src/type/__tests__/introspection-test.ts b/src/type/__tests__/introspection-test.ts index 1fa0518dd0..93c41e810f 100644 --- a/src/type/__tests__/introspection-test.ts +++ b/src/type/__tests__/introspection-test.ts @@ -848,6 +848,11 @@ describe('Introspection', () => { isDeprecated: false, deprecationReason: null, }, + { + name: 'FRAGMENT_VARIABLE_DEFINITION', + isDeprecated: false, + deprecationReason: null, + }, { name: 'SCHEMA', isDeprecated: false, diff --git a/src/type/introspection.ts b/src/type/introspection.ts index aedce3e6a8..808839911f 100644 --- a/src/type/introspection.ts +++ b/src/type/introspection.ts @@ -155,7 +155,11 @@ export const __DirectiveLocation: GraphQLEnumType = new GraphQLEnumType({ }, VARIABLE_DEFINITION: { value: DirectiveLocation.VARIABLE_DEFINITION, - description: 'Location adjacent to a variable definition.', + description: 'Location adjacent to an operation variable definition.', + }, + FRAGMENT_VARIABLE_DEFINITION: { + value: DirectiveLocation.FRAGMENT_VARIABLE_DEFINITION, + description: 'Location adjacent to a fragment variable definition.', }, SCHEMA: { value: DirectiveLocation.SCHEMA, diff --git a/src/utilities/__tests__/printSchema-test.ts b/src/utilities/__tests__/printSchema-test.ts index 29799a4881..e2d082b69c 100644 --- a/src/utilities/__tests__/printSchema-test.ts +++ b/src/utilities/__tests__/printSchema-test.ts @@ -831,9 +831,12 @@ describe('Type System Printer', () => { """Location adjacent to an inline fragment.""" INLINE_FRAGMENT - """Location adjacent to a variable definition.""" + """Location adjacent to an operation variable definition.""" VARIABLE_DEFINITION + """Location adjacent to a fragment variable definition.""" + FRAGMENT_VARIABLE_DEFINITION + """Location adjacent to a schema definition.""" SCHEMA diff --git a/src/utilities/buildASTSchema.ts b/src/utilities/buildASTSchema.ts index 09fd7cd7dd..5be0b6e421 100644 --- a/src/utilities/buildASTSchema.ts +++ b/src/utilities/buildASTSchema.ts @@ -93,7 +93,7 @@ export function buildSchema( ): GraphQLSchema { const document = parse(source, { noLocation: options?.noLocation, - allowLegacyFragmentVariables: options?.allowLegacyFragmentVariables, + experimentalFragmentArguments: options?.experimentalFragmentArguments, }); return buildASTSchema(document, { diff --git a/src/validation/__tests__/KnownDirectivesRule-test.ts b/src/validation/__tests__/KnownDirectivesRule-test.ts index a3bbc198da..c27155c9b2 100644 --- a/src/validation/__tests__/KnownDirectivesRule-test.ts +++ b/src/validation/__tests__/KnownDirectivesRule-test.ts @@ -42,6 +42,7 @@ const schemaWithDirectives = buildSchema(` directive @onField on FIELD directive @onFragmentDefinition on FRAGMENT_DEFINITION directive @onFragmentSpread on FRAGMENT_SPREAD + directive @onFragmentVariableDefinition on FRAGMENT_VARIABLE_DEFINITION directive @onInlineFragment on INLINE_FRAGMENT directive @onVariableDefinition on VARIABLE_DEFINITION `); @@ -150,7 +151,9 @@ describe('Validate: Known directives', () => { someField @onField } - fragment Frag on Human @onFragmentDefinition { + fragment Frag( + $arg: Int @onFragmentVariableDefinition + ) on Human @onFragmentDefinition { name @onField } `); @@ -175,7 +178,7 @@ describe('Validate: Known directives', () => { someField @onQuery } - fragment Frag on Human @onQuery { + fragment Frag($arg: Int @onVariableDefinition) on Human @onQuery { name @onQuery } `).toDeepEqual([ @@ -219,9 +222,14 @@ describe('Validate: Known directives', () => { message: 'Directive "@onQuery" may not be used on FIELD.', locations: [{ column: 19, line: 16 }], }, + { + message: + 'Directive "@onVariableDefinition" may not be used on FRAGMENT_VARIABLE_DEFINITION.', + locations: [{ column: 31, line: 19 }], + }, { message: 'Directive "@onQuery" may not be used on FRAGMENT_DEFINITION.', - locations: [{ column: 30, line: 19 }], + locations: [{ column: 63, line: 19 }], }, { message: 'Directive "@onQuery" may not be used on FIELD.', diff --git a/src/validation/__tests__/harness.ts b/src/validation/__tests__/harness.ts index 682932d897..7f478fddfe 100644 --- a/src/validation/__tests__/harness.ts +++ b/src/validation/__tests__/harness.ts @@ -123,7 +123,7 @@ export function expectValidationErrorsWithSchema( rule: ValidationRule, queryStr: string, ): any { - const doc = parse(queryStr); + const doc = parse(queryStr, { experimentalFragmentArguments: true }); const errors = validate(schema, doc, [rule]); return expectJSON(errors); } diff --git a/src/validation/rules/KnownDirectivesRule.ts b/src/validation/rules/KnownDirectivesRule.ts index 57641b91e7..c31da7a7a2 100644 --- a/src/validation/rules/KnownDirectivesRule.ts +++ b/src/validation/rules/KnownDirectivesRule.ts @@ -86,8 +86,14 @@ function getDirectiveLocationForASTPath( return DirectiveLocation.INLINE_FRAGMENT; case Kind.FRAGMENT_DEFINITION: return DirectiveLocation.FRAGMENT_DEFINITION; - case Kind.VARIABLE_DEFINITION: - return DirectiveLocation.VARIABLE_DEFINITION; + case Kind.VARIABLE_DEFINITION: { + const parentNode = ancestors[ancestors.length - 3]; + invariant('kind' in parentNode); + return parentNode.kind === Kind.OPERATION_DEFINITION + ? DirectiveLocation.VARIABLE_DEFINITION + : DirectiveLocation.FRAGMENT_VARIABLE_DEFINITION; + } + case Kind.SCHEMA_DEFINITION: case Kind.SCHEMA_EXTENSION: return DirectiveLocation.SCHEMA;