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 7eea508458..8669b64ec7 100644 --- a/src/language/__tests__/printer-test.ts +++ b/src/language/__tests__/printer-test.ts @@ -168,34 +168,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 c70cb783d5..ab44b1a518 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('properly visits the kitchen sink query', () => { const ast = parse(kitchenSinkQuery, { experimentalClientControlledNullability: true, diff --git a/src/language/ast.ts b/src/language/ast.ts index 22a7cc253c..c42866119d 100644 --- a/src/language/ast.ts +++ b/src/language/ast.ts @@ -227,7 +227,7 @@ export const QueryDocumentKeys: { NonNullAssertion: ['nullabilityAssertion'], ErrorBoundary: ['nullabilityAssertion'], - FragmentSpread: ['name', 'directives'], + FragmentSpread: ['name', 'arguments', 'directives'], InlineFragment: ['typeCondition', 'directives', 'selectionSet'], FragmentDefinition: [ 'name', @@ -428,6 +428,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 +444,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..568de6bba1 100644 --- a/src/language/directiveLocation.ts +++ b/src/language/directiveLocation.ts @@ -23,4 +23,5 @@ export enum DirectiveLocation { ENUM_VALUE = 'ENUM_VALUE', INPUT_OBJECT = 'INPUT_OBJECT', INPUT_FIELD_DEFINITION = 'INPUT_FIELD_DEFINITION', + FRAGMENT_VARIABLE_DEFINITION = 'FRAGMENT_VARIABLE_DEFINITION', } diff --git a/src/language/parser.ts b/src/language/parser.ts index cd9345f6dd..f05a4803e0 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,21 +592,18 @@ 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) { + if (this._options.experimentalFragmentArguments === true) { return this.node(start, { kind: Kind.FRAGMENT_DEFINITION, name: this.parseFragmentName(), - variableDefinitions: this.parseVariableDefinitions(), + variableDefinitions:this.parseVariableDefinitions(), 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 28bb25da17..bd82672735 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: { @@ -392,3 +391,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 e6e5d0943e..160c2ea245 100644 --- a/src/type/__tests__/introspection-test.ts +++ b/src/type/__tests__/introspection-test.ts @@ -859,6 +859,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 5f3c9c1fa5..6d5c29dd90 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 78f793b183..ac9002e388 100644 --- a/src/utilities/__tests__/printSchema-test.ts +++ b/src/utilities/__tests__/printSchema-test.ts @@ -971,9 +971,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..2cc8c44ede 100644 --- a/src/validation/__tests__/KnownDirectivesRule-test.ts +++ b/src/validation/__tests__/KnownDirectivesRule-test.ts @@ -44,6 +44,7 @@ const schemaWithDirectives = buildSchema(` directive @onFragmentSpread on FRAGMENT_SPREAD directive @onInlineFragment on INLINE_FRAGMENT directive @onVariableDefinition on VARIABLE_DEFINITION + directive @onFragmentVariableDefinition on FRAGMENT_VARIABLE_DEFINITION `); const schemaWithSDLDirectives = buildSchema(` @@ -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 b7710ff9d9..0db861f45b 100644 --- a/src/validation/__tests__/harness.ts +++ b/src/validation/__tests__/harness.ts @@ -129,7 +129,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 a2c7ec81eb..9a1e87d029 100644 --- a/src/validation/rules/KnownDirectivesRule.ts +++ b/src/validation/rules/KnownDirectivesRule.ts @@ -89,8 +89,13 @@ 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;