Skip to content

Commit ce1ff75

Browse files
mjmahonemjmahone
committed
Re-apply changes, address olds comments, but do not start on functional work
1 parent f3b0fa6 commit ce1ff75

File tree

11 files changed

+308
-144
lines changed

11 files changed

+308
-144
lines changed

‎src/execution/__tests__/variables-test.ts

Lines changed: 150 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1006,6 +1006,156 @@ describe('Execute: Handles inputs', () => {
10061006
});
10071007
});
10081008

1009+
describe('using fragment arguments', () => {
1010+
it('when there are no fragment arguments', () => {
1011+
const result = executeQuery(`
1012+
query {
1013+
...a
1014+
}
1015+
fragment a on TestType {
1016+
fieldWithNonNullableStringInput(input: "A")
1017+
}
1018+
`);
1019+
expect(result).to.deep.equal({
1020+
data: {
1021+
fieldWithNonNullableStringInput: '"A"',
1022+
},
1023+
});
1024+
});
1025+
1026+
it('when a value is required and provided', () => {
1027+
const result = executeQuery(`
1028+
query {
1029+
...a(value: "A")
1030+
}
1031+
fragment a($value: String!) on TestType {
1032+
fieldWithNonNullableStringInput(input: $value)
1033+
}
1034+
`);
1035+
expect(result).to.deep.equal({
1036+
data: {
1037+
fieldWithNonNullableStringInput: '"A"',
1038+
},
1039+
});
1040+
});
1041+
1042+
it('when a value is required and not provided', () => {
1043+
const result = executeQuery(`
1044+
query {
1045+
...a
1046+
}
1047+
fragment a($value: String!) on TestType {
1048+
fieldWithNullableStringInput(input: $value)
1049+
}
1050+
`);
1051+
expect(result).to.deep.equal({
1052+
data: {
1053+
fieldWithNullableStringInput: null,
1054+
},
1055+
});
1056+
});
1057+
1058+
it('when the definition has a default and is provided', () => {
1059+
const result = executeQuery(`
1060+
query {
1061+
...a(value: "A")
1062+
}
1063+
fragment a($value: String! = "B") on TestType {
1064+
fieldWithNonNullableStringInput(input: $value)
1065+
}
1066+
`);
1067+
expect(result).to.deep.equal({
1068+
data: {
1069+
fieldWithNonNullableStringInput: '"A"',
1070+
},
1071+
});
1072+
});
1073+
1074+
it('when the definition has a default and is not provided', () => {
1075+
const result = executeQuery(`
1076+
query {
1077+
...a
1078+
}
1079+
fragment a($value: String! = "B") on TestType {
1080+
fieldWithNonNullableStringInput(input: $value)
1081+
}
1082+
`);
1083+
expect(result).to.deep.equal({
1084+
data: {
1085+
fieldWithNonNullableStringInput: '"B"',
1086+
},
1087+
});
1088+
});
1089+
1090+
it('when the definition has a non-nullable default and is provided null', () => {
1091+
const result = executeQuery(`
1092+
query {
1093+
...a(value: null)
1094+
}
1095+
fragment a($value: String! = "B") on TestType {
1096+
fieldWithNullableStringInput(input: $value)
1097+
}
1098+
`);
1099+
expect(result).to.deep.equal({
1100+
data: {
1101+
fieldWithNullableStringInput: 'null',
1102+
},
1103+
});
1104+
});
1105+
1106+
it('when the definition has no default and is not provided', () => {
1107+
const result = executeQuery(`
1108+
query {
1109+
...a
1110+
}
1111+
fragment a($value: String) on TestType {
1112+
fieldWithNonNullableStringInputAndDefaultArgumentValue(input: $value)
1113+
}
1114+
`);
1115+
expect(result).to.deep.equal({
1116+
data: {
1117+
fieldWithNonNullableStringInputAndDefaultArgumentValue:
1118+
'"Hello World"',
1119+
},
1120+
});
1121+
});
1122+
1123+
it('when the argument variable is nested in a complex type', () => {
1124+
const result = executeQuery(`
1125+
query {
1126+
...a(value: "C")
1127+
}
1128+
fragment a($value: String) on TestType {
1129+
list(input: ["A", "B", $value, "D"])
1130+
}
1131+
`);
1132+
expect(result).to.deep.equal({
1133+
data: {
1134+
list: '["A", "B", "C", "D"]',
1135+
},
1136+
});
1137+
});
1138+
1139+
it('when argument variables are used recursively', () => {
1140+
const result = executeQuery(`
1141+
query {
1142+
...a(aValue: "C")
1143+
}
1144+
fragment a($aValue: String) on TestType {
1145+
...b(bValue: $aValue)
1146+
}
1147+
fragment b($bValue: String) on TestType {
1148+
list(input: ["A", "B", $bValue, "D"])
1149+
}
1150+
`);
1151+
expect(result).to.deep.equal({
1152+
data: {
1153+
list: '["A", "B", "C", "D"]',
1154+
},
1155+
});
1156+
});
1157+
});
1158+
10091159
describe('getVariableValues: limit maximum number of coercion errors', () => {
10101160
const doc = parse(`
10111161
query ($input: [String!]) {

‎src/execution/collectFields.ts

Lines changed: 46 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -2,13 +2,17 @@ import { AccumulatorMap } from '../jsutils/AccumulatorMap';
22
import type { ObjMap } from '../jsutils/ObjMap';
33

44
import type {
5+
ArgumentNode,
56
FieldNode,
67
FragmentDefinitionNode,
78
FragmentSpreadNode,
89
InlineFragmentNode,
910
SelectionSetNode,
11+
ValueNode,
12+
VariableDefinitionNode,
1013
} from '../language/ast';
1114
import { Kind } from '../language/kinds';
15+
import { visit } from '../language/visitor';
1216

1317
import type { GraphQLObjectType } from '../type/definition';
1418
import { isAbstractType } from '../type/definition';
@@ -139,12 +143,18 @@ function collectFieldsImpl(
139143
) {
140144
continue;
141145
}
146+
const selectionSetWithAppliedArguments =
147+
selectionSetWithFragmentArgumentsApplied(
148+
fragment.argumentDefinitions,
149+
selection.arguments,
150+
fragment.selectionSet,
151+
);
142152
collectFieldsImpl(
143153
schema,
144154
fragments,
145155
variableValues,
146156
runtimeType,
147-
fragment.selectionSet,
157+
selectionSetWithAppliedArguments,
148158
fields,
149159
visitedFragmentNames,
150160
);
@@ -154,6 +164,41 @@ function collectFieldsImpl(
154164
}
155165
}
156166

167+
function selectionSetWithFragmentArgumentsApplied(
168+
argumentDefinitions: Readonly<Array<VariableDefinitionNode>> | undefined,
169+
fragmentArguments: Readonly<Array<ArgumentNode>> | undefined,
170+
selectionSet: SelectionSetNode,
171+
): SelectionSetNode {
172+
const providedArguments = new Map<string, ArgumentNode>();
173+
if (fragmentArguments) {
174+
for (const argument of fragmentArguments) {
175+
providedArguments.set(argument.name.value, argument);
176+
}
177+
}
178+
179+
const fragmentArgumentValues = new Map<string, ValueNode>();
180+
if (argumentDefinitions) {
181+
for (const argumentDef of argumentDefinitions) {
182+
const variableName = argumentDef.variable.name.value;
183+
const providedArg = providedArguments.get(variableName);
184+
if (providedArg) {
185+
// Not valid if the providedArg is null and argumentDef is non-null
186+
fragmentArgumentValues.set(variableName, providedArg.value);
187+
} else if (argumentDef.defaultValue) {
188+
fragmentArgumentValues.set(variableName, argumentDef.defaultValue);
189+
}
190+
// If argumentDef is non-null, expect a provided arg or non-null default value.
191+
// Otherwise just preserve the variable as-is: it will be treated as unset by the executor.
192+
}
193+
}
194+
195+
return visit(selectionSet, {
196+
Variable(node) {
197+
return fragmentArgumentValues.get(node.name.value);
198+
},
199+
});
200+
}
201+
157202
/**
158203
* Determines if a field should be included based on the `@include` and `@skip`
159204
* directives, where `@skip` has higher precedence than `@include`.

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

Lines changed: 8 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -591,13 +591,16 @@ describe('Parser', () => {
591591
expect('loc' in result).to.equal(false);
592592
});
593593

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

597-
expect(() =>
598-
parse(document, { allowLegacyFragmentVariables: true }),
599-
).to.not.throw();
600-
expect(() => parse(document)).to.throw('Syntax Error');
597+
expect(() => parse(document)).to.not.throw();
598+
});
599+
600+
it('allows parsing fragment spread arguments', () => {
601+
const document = 'fragment a on t { ...b(v: $v) }';
602+
603+
expect(() => parse(document)).to.not.throw();
601604
});
602605

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

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

Lines changed: 32 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -110,34 +110,58 @@ 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 },
117116
);
118-
expect(print(queryASTWithVariableDirective)).to.equal(dedent`
117+
expect(print(fragmentWithArgumentDefinitionDirective)).to.equal(dedent`
119118
fragment Foo($foo: TestType @test) on TestType @testDirective {
120119
id
121120
}
122121
`);
123122
});
124123

125-
it('Legacy: correctly prints fragment defined variables', () => {
126-
const fragmentWithVariable = parse(
124+
it('correctly prints fragment defined arguments', () => {
125+
const fragmentWithArgumentDefinition = parse(
127126
`
128127
fragment Foo($a: ComplexType, $b: Boolean = false) on TestType {
129128
id
130129
}
131130
`,
132-
{ allowLegacyFragmentVariables: true },
133131
);
134-
expect(print(fragmentWithVariable)).to.equal(dedent`
132+
expect(print(fragmentWithArgumentDefinition)).to.equal(dedent`
135133
fragment Foo($a: ComplexType, $b: Boolean = false) on TestType {
136134
id
137135
}
138136
`);
139137
});
140138

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

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

Lines changed: 44 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -455,10 +455,9 @@ 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,
462461
});
463462
const visited: Array<any> = [];
464463

@@ -505,6 +504,49 @@ describe('Visitor', () => {
505504
]);
506505
});
507506

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

0 commit comments

Comments
 (0)