Fragment Arguments: Only Syntax Changes

mjmahone · mjmahone · commit 5fd9f21fca14 · 2023-02-09T16:09:43.000-05:00
diff --git 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
@@ -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
@@ -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<any> = [];
 
@@ -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<any> = [];
+
+    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
@@ -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<ArgumentNode> | undefined;
   readonly directives?: ReadonlyArray<DirectiveNode> | 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<VariableDefinitionNode>
     | undefined;
diff --git 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/kinds.ts b/src/language/kinds.ts
@@ -22,6 +22,7 @@ export enum Kind {
   FRAGMENT_SPREAD = 'FragmentSpread',
   INLINE_FRAGMENT = 'InlineFragment',
   FRAGMENT_DEFINITION = 'FragmentDefinition',
+  FRAGMENT_ARGUMENT_DEFINITION = 'FragmentArgumentDefinition',
 
   /** Values */
   VARIABLE = 'Variable',
diff --git 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<FragmentSpreadNode>(start, {
+          kind: Kind.FRAGMENT_SPREAD,
+          name,
+          arguments: this.parseArguments(false),
+          directives: this.parseDirectives(false),
+        });
+      }
       return this.node<FragmentSpreadNode>(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<FragmentDefinitionNode>(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<FragmentDefinitionNode>(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
@@ -64,14 +64,9 @@ const printDocASTReducer: ASTReducer<string> = {
       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,10 @@ const printDocASTReducer: ASTReducer<string> = {
   // 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 +375,12 @@ function hasMultilineItems(maybeArray: Maybe<ReadonlyArray<string>>): boolean {
   /* c8 ignore next */
   return maybeArray?.some((str) => str.includes('\n')) ?? false;
 }
+
+function wrappedLineAndArgs(prefix: string, args: ReadonlyArray<string> | 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
@@ -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
@@ -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
@@ -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
diff --git a/src/validation/__tests__/KnownDirectivesRule-test.ts b/src/validation/__tests__/KnownDirectivesRule-test.ts
diff --git a/src/validation/__tests__/harness.ts b/src/validation/__tests__/harness.ts
diff --git a/src/validation/rules/KnownDirectivesRule.ts b/src/validation/rules/KnownDirectivesRule.ts
