Skip to content

Commit c845d38

Browse files
mjmahonemjmahone
committed
Fragment Arguments: Only Syntax Changes
1 parent f201681 commit c845d38

File tree

15 files changed

+191
-56
lines changed

15 files changed

+191
-56
lines changed

‎src/language/__tests__/parser-test.ts

Lines changed: 22 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -607,13 +607,32 @@ describe('Parser', () => {
607607
expect('loc' in result).to.equal(false);
608608
});
609609

610-
it('Legacy: allows parsing fragment defined variables', () => {
610+
it('allows parsing fragment defined variables', () => {
611611
const document = 'fragment a($v: Boolean = false) on t { f(v: $v) }';
612612

613613
expect(() =>
614-
parse(document, { allowLegacyFragmentVariables: true }),
614+
parse(document, { experimentalFragmentArguments: true }),
615615
).to.not.throw();
616-
expect(() => parse(document)).to.throw('Syntax Error');
616+
});
617+
618+
it('disallows parsing fragment defined variables without experimental flag', () => {
619+
const document = 'fragment a($v: Boolean = false) on t { f(v: $v) }';
620+
621+
expect(() => parse(document)).to.throw();
622+
});
623+
624+
it('allows parsing fragment spread arguments', () => {
625+
const document = 'fragment a on t { ...b(v: $v) }';
626+
627+
expect(() =>
628+
parse(document, { experimentalFragmentArguments: true }),
629+
).to.not.throw();
630+
});
631+
632+
it('disallows parsing fragment spread arguments without experimental flag', () => {
633+
const document = 'fragment a on t { ...b(v: $v) }';
634+
635+
expect(() => parse(document)).to.throw();
617636
});
618637

619638
it('contains location that can be Object.toStringified, JSON.stringified, or jsutils.inspected', () => {

‎src/language/__tests__/printer-test.ts

Lines changed: 36 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -110,34 +110,62 @@ describe('Printer: Query document', () => {
110110
`);
111111
});
112112

113-
it('Legacy: prints fragment with variable directives', () => {
114-
const queryASTWithVariableDirective = parse(
113+
it('prints fragment with argument definition directives', () => {
114+
const fragmentWithArgumentDefinitionDirective = parse(
115115
'fragment Foo($foo: TestType @test) on TestType @testDirective { id }',
116-
{ allowLegacyFragmentVariables: true },
116+
{ experimentalFragmentArguments: true },
117117
);
118-
expect(print(queryASTWithVariableDirective)).to.equal(dedent`
118+
expect(print(fragmentWithArgumentDefinitionDirective)).to.equal(dedent`
119119
fragment Foo($foo: TestType @test) on TestType @testDirective {
120120
id
121121
}
122122
`);
123123
});
124124

125-
it('Legacy: correctly prints fragment defined variables', () => {
126-
const fragmentWithVariable = parse(
125+
it('correctly prints fragment defined arguments', () => {
126+
const fragmentWithArgumentDefinition = parse(
127127
`
128128
fragment Foo($a: ComplexType, $b: Boolean = false) on TestType {
129129
id
130130
}
131131
`,
132-
{ allowLegacyFragmentVariables: true },
132+
{ experimentalFragmentArguments: true },
133133
);
134-
expect(print(fragmentWithVariable)).to.equal(dedent`
134+
expect(print(fragmentWithArgumentDefinition)).to.equal(dedent`
135135
fragment Foo($a: ComplexType, $b: Boolean = false) on TestType {
136136
id
137137
}
138138
`);
139139
});
140140

141+
it('prints fragment spread with arguments', () => {
142+
const fragmentSpreadWithArguments = parse(
143+
'fragment Foo on TestType { ...Bar(a: {x: $x}, b: true) }',
144+
{ experimentalFragmentArguments: true },
145+
);
146+
expect(print(fragmentSpreadWithArguments)).to.equal(dedent`
147+
fragment Foo on TestType {
148+
...Bar(a: { x: $x }, b: true)
149+
}
150+
`);
151+
});
152+
153+
it('prints fragment spread with multi-line arguments', () => {
154+
const fragmentSpreadWithArguments = parse(
155+
'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") }',
156+
{ experimentalFragmentArguments: true },
157+
);
158+
expect(print(fragmentSpreadWithArguments)).to.equal(dedent`
159+
fragment Foo on TestType {
160+
...Bar(
161+
a: { x: $x, y: $y, z: $z, xy: $xy }
162+
b: true
163+
c: "a long string extending arguments over max length"
164+
)
165+
}
166+
`);
167+
});
168+
141169
it('prints kitchen sink without altering ast', () => {
142170
const ast = parse(kitchenSinkQuery, {
143171
noLocation: true,

‎src/language/__tests__/visitor-test.ts

Lines changed: 46 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -455,10 +455,10 @@ describe('Visitor', () => {
455455
]);
456456
});
457457

458-
it('Legacy: visits variables defined in fragments', () => {
458+
it('visits arguments defined on fragments', () => {
459459
const ast = parse('fragment a($v: Boolean = false) on t { f }', {
460460
noLocation: true,
461-
allowLegacyFragmentVariables: true,
461+
experimentalFragmentArguments: true,
462462
});
463463
const visited: Array<any> = [];
464464

@@ -505,6 +505,50 @@ describe('Visitor', () => {
505505
]);
506506
});
507507

508+
it('visits arguments on fragment spreads', () => {
509+
const ast = parse('fragment a on t { ...s(v: false) }', {
510+
noLocation: true,
511+
experimentalFragmentArguments: true,
512+
});
513+
const visited: Array<any> = [];
514+
515+
visit(ast, {
516+
enter(node) {
517+
checkVisitorFnArgs(ast, arguments);
518+
visited.push(['enter', node.kind, getValue(node)]);
519+
},
520+
leave(node) {
521+
checkVisitorFnArgs(ast, arguments);
522+
visited.push(['leave', node.kind, getValue(node)]);
523+
},
524+
});
525+
526+
expect(visited).to.deep.equal([
527+
['enter', 'Document', undefined],
528+
['enter', 'FragmentDefinition', undefined],
529+
['enter', 'Name', 'a'],
530+
['leave', 'Name', 'a'],
531+
['enter', 'NamedType', undefined],
532+
['enter', 'Name', 't'],
533+
['leave', 'Name', 't'],
534+
['leave', 'NamedType', undefined],
535+
['enter', 'SelectionSet', undefined],
536+
['enter', 'FragmentSpread', undefined],
537+
['enter', 'Name', 's'],
538+
['leave', 'Name', 's'],
539+
['enter', 'Argument', { kind: 'BooleanValue', value: false }],
540+
['enter', 'Name', 'v'],
541+
['leave', 'Name', 'v'],
542+
['enter', 'BooleanValue', false],
543+
['leave', 'BooleanValue', false],
544+
['leave', 'Argument', { kind: 'BooleanValue', value: false }],
545+
['leave', 'FragmentSpread', undefined],
546+
['leave', 'SelectionSet', undefined],
547+
['leave', 'FragmentDefinition', undefined],
548+
['leave', 'Document', undefined],
549+
]);
550+
});
551+
508552
it('n', () => {
509553
const ast = parse(kitchenSinkQuery, {
510554
experimentalClientControlledNullability: true,

‎src/language/ast.ts

Lines changed: 2 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -227,11 +227,10 @@ export const QueryDocumentKeys: {
227227
NonNullAssertion: ['nullabilityAssertion'],
228228
ErrorBoundary: ['nullabilityAssertion'],
229229

230-
FragmentSpread: ['name', 'directives'],
230+
FragmentSpread: ['name', 'arguments', 'directives'],
231231
InlineFragment: ['typeCondition', 'directives', 'selectionSet'],
232232
FragmentDefinition: [
233233
'name',
234-
// Note: fragment variable definitions are deprecated and will removed in v17.0.0
235234
'variableDefinitions',
236235
'typeCondition',
237236
'directives',
@@ -428,6 +427,7 @@ export interface FragmentSpreadNode {
428427
readonly kind: Kind.FRAGMENT_SPREAD;
429428
readonly loc?: Location | undefined;
430429
readonly name: NameNode;
430+
readonly arguments?: ReadonlyArray<ArgumentNode> | undefined;
431431
readonly directives?: ReadonlyArray<DirectiveNode> | undefined;
432432
}
433433

@@ -443,7 +443,6 @@ export interface FragmentDefinitionNode {
443443
readonly kind: Kind.FRAGMENT_DEFINITION;
444444
readonly loc?: Location | undefined;
445445
readonly name: NameNode;
446-
/** @deprecated variableDefinitions will be removed in v17.0.0 */
447446
readonly variableDefinitions?:
448447
| ReadonlyArray<VariableDefinitionNode>
449448
| undefined;

‎src/language/directiveLocation.ts

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -11,6 +11,7 @@ export enum DirectiveLocation {
1111
FRAGMENT_SPREAD = 'FRAGMENT_SPREAD',
1212
INLINE_FRAGMENT = 'INLINE_FRAGMENT',
1313
VARIABLE_DEFINITION = 'VARIABLE_DEFINITION',
14+
FRAGMENT_VARIABLE_DEFINITION = 'FRAGMENT_VARIABLE_DEFINITION',
1415
/** Type System Definitions */
1516
SCHEMA = 'SCHEMA',
1617
SCALAR = 'SCALAR',

‎src/language/kinds.ts

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -22,6 +22,7 @@ export enum Kind {
2222
FRAGMENT_SPREAD = 'FragmentSpread',
2323
INLINE_FRAGMENT = 'InlineFragment',
2424
FRAGMENT_DEFINITION = 'FragmentDefinition',
25+
FRAGMENT_ARGUMENT_DEFINITION = 'FragmentArgumentDefinition',
2526

2627
/** Values */
2728
VARIABLE = 'Variable',

‎src/language/parser.ts

Lines changed: 29 additions & 23 deletions
Original file line numberDiff line numberDiff line change
@@ -92,21 +92,25 @@ export interface ParseOptions {
9292
maxTokens?: number | undefined;
9393

9494
/**
95-
* @deprecated will be removed in the v17.0.0
95+
* EXPERIMENTAL:
9696
*
97-
* If enabled, the parser will understand and parse variable definitions
98-
* contained in a fragment definition. They'll be represented in the
99-
* `variableDefinitions` field of the FragmentDefinitionNode.
97+
* If enabled, the parser will understand and parse fragment variable definitions
98+
* and arguments on fragment spreads. Fragment variable definitions will be represented
99+
* in the `variableDefinitions` field of the FragmentDefinitionNode.
100+
* Fragment spread arguments will be represented in the `arguments` field of FragmentSpreadNode.
100101
*
101-
* The syntax is identical to normal, query-defined variables. For example:
102+
* For example:
102103
*
103104
* ```graphql
105+
* {
106+
* t { ...A(var: true) }
107+
* }
104108
* fragment A($var: Boolean = false) on T {
105-
* ...
109+
* ...B(x: $var)
106110
* }
107111
* ```
108112
*/
109-
allowLegacyFragmentVariables?: boolean | undefined;
113+
experimentalFragmentArguments?: boolean | undefined;
110114

111115
/**
112116
* EXPERIMENTAL:
@@ -550,7 +554,7 @@ export class Parser {
550554
/**
551555
* Corresponds to both FragmentSpread and InlineFragment in the spec.
552556
*
553-
* FragmentSpread : ... FragmentName Directives?
557+
* FragmentSpread : ... FragmentName Arguments? Directives?
554558
*
555559
* InlineFragment : ... TypeCondition? Directives? SelectionSet
556560
*/
@@ -560,9 +564,21 @@ export class Parser {
560564

561565
const hasTypeCondition = this.expectOptionalKeyword('on');
562566
if (!hasTypeCondition && this.peek(TokenKind.NAME)) {
567+
const name = this.parseFragmentName();
568+
if (
569+
this.peek(TokenKind.PAREN_L) &&
570+
this._options.experimentalFragmentArguments
571+
) {
572+
return this.node<FragmentSpreadNode>(start, {
573+
kind: Kind.FRAGMENT_SPREAD,
574+
name,
575+
arguments: this.parseArguments(false),
576+
directives: this.parseDirectives(false),
577+
});
578+
}
563579
return this.node<FragmentSpreadNode>(start, {
564580
kind: Kind.FRAGMENT_SPREAD,
565-
name: this.parseFragmentName(),
581+
name,
566582
directives: this.parseDirectives(false),
567583
});
568584
}
@@ -576,29 +592,19 @@ export class Parser {
576592

577593
/**
578594
* FragmentDefinition :
579-
* - fragment FragmentName on TypeCondition Directives? SelectionSet
595+
* - fragment FragmentName VariableDefinitions? on TypeCondition Directives? SelectionSet
580596
*
581597
* TypeCondition : NamedType
582598
*/
583599
parseFragmentDefinition(): FragmentDefinitionNode {
584600
const start = this._lexer.token;
585601
this.expectKeyword('fragment');
586-
// Legacy support for defining variables within fragments changes
587-
// the grammar of FragmentDefinition:
588-
// - fragment FragmentName VariableDefinitions? on TypeCondition Directives? SelectionSet
589-
if (this._options.allowLegacyFragmentVariables === true) {
590-
return this.node<FragmentDefinitionNode>(start, {
591-
kind: Kind.FRAGMENT_DEFINITION,
592-
name: this.parseFragmentName(),
593-
variableDefinitions: this.parseVariableDefinitions(),
594-
typeCondition: (this.expectKeyword('on'), this.parseNamedType()),
595-
directives: this.parseDirectives(false),
596-
selectionSet: this.parseSelectionSet(),
597-
});
598-
}
599602
return this.node<FragmentDefinitionNode>(start, {
600603
kind: Kind.FRAGMENT_DEFINITION,
601604
name: this.parseFragmentName(),
605+
variableDefinitions: this._options.experimentalFragmentArguments
606+
? this.parseVariableDefinitions()
607+
: undefined,
602608
typeCondition: (this.expectKeyword('on'), this.parseNamedType()),
603609
directives: this.parseDirectives(false),
604610
selectionSet: this.parseSelectionSet(),

‎src/language/printer.ts

Lines changed: 19 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -64,14 +64,9 @@ const printDocASTReducer: ASTReducer<string> = {
6464
selectionSet,
6565
}) {
6666
const prefix = join([wrap('', alias, ': '), name], '');
67-
let argsLine = prefix + wrap('(', join(args, ', '), ')');
68-
69-
if (argsLine.length > MAX_LINE_LENGTH) {
70-
argsLine = prefix + wrap('(\n', indent(join(args, '\n')), '\n)');
71-
}
7267

7368
return join([
74-
argsLine,
69+
wrappedLineAndArgs(prefix, args),
7570
// Note: Client Controlled Nullability is experimental and may be
7671
// changed or removed in the future.
7772
nullabilityAssertion,
@@ -105,8 +100,12 @@ const printDocASTReducer: ASTReducer<string> = {
105100
// Fragments
106101

107102
FragmentSpread: {
108-
leave: ({ name, directives }) =>
109-
'...' + name + wrap(' ', join(directives, ' ')),
103+
leave: ({ name, arguments: args, directives }) => {
104+
const prefix = '...' + name;
105+
return (
106+
wrappedLineAndArgs(prefix, args) + wrap(' ', join(directives, ' '))
107+
);
108+
},
110109
},
111110

112111
InlineFragment: {
@@ -378,3 +377,15 @@ function hasMultilineItems(maybeArray: Maybe<ReadonlyArray<string>>): boolean {
378377
/* c8 ignore next */
379378
return maybeArray?.some((str) => str.includes('\n')) ?? false;
380379
}
380+
381+
function wrappedLineAndArgs(
382+
prefix: string,
383+
args: ReadonlyArray<string> | undefined,
384+
): string {
385+
let argsLine = prefix + wrap('(', join(args, ', '), ')');
386+
387+
if (argsLine.length > MAX_LINE_LENGTH) {
388+
argsLine = prefix + wrap('(\n', indent(join(args, '\n')), '\n)');
389+
}
390+
return argsLine;
391+
}

‎src/type/__tests__/introspection-test.ts

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -848,6 +848,11 @@ describe('Introspection', () => {
848848
isDeprecated: false,
849849
deprecationReason: null,
850850
},
851+
{
852+
name: 'FRAGMENT_VARIABLE_DEFINITION',
853+
isDeprecated: false,
854+
deprecationReason: null,
855+
},
851856
{
852857
name: 'SCHEMA',
853858
isDeprecated: false,

‎src/type/introspection.ts

Lines changed: 5 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -155,7 +155,11 @@ export const __DirectiveLocation: GraphQLEnumType = new GraphQLEnumType({
155155
},
156156
VARIABLE_DEFINITION: {
157157
value: DirectiveLocation.VARIABLE_DEFINITION,
158-
description: 'Location adjacent to a variable definition.',
158+
description: 'Location adjacent to an operation variable definition.',
159+
},
160+
FRAGMENT_VARIABLE_DEFINITION: {
161+
value: DirectiveLocation.FRAGMENT_VARIABLE_DEFINITION,
162+
description: 'Location adjacent to a fragment variable definition.',
159163
},
160164
SCHEMA: {
161165
value: DirectiveLocation.SCHEMA,

0 commit comments

Comments
 (0)