Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Fragment Arguments Reference Implementation #3835

Closed
wants to merge 4 commits into from
Closed

Fragment Arguments Reference Implementation #3835

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
4b1ae03
Fragment Arguments: Only Syntax Changes
mjmahone Feb 9, 2023
e9fbda2
RFC: Fragment Arguments
mjmahone Jan 19, 2023
bfd80a6
Add flag for parser
mjmahone Feb 9, 2023
7fe0733
Split PRs, no functional changes
mjmahone Feb 9, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
252 changes: 251 additions & 1 deletion src/execution/__tests__/variables-test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -104,6 +104,13 @@ function fieldWithInputArg(
};
}

const NestedType: GraphQLObjectType = new GraphQLObjectType({
name: 'NestedType',
fields: {
echo: fieldWithInputArg({ type: GraphQLString }),
},
});

const TestType = new GraphQLObjectType({
name: 'TestType',
fields: {
Expand All @@ -129,6 +136,10 @@ const TestType = new GraphQLObjectType({
defaultValue: 'Hello World',
}),
list: fieldWithInputArg({ type: new GraphQLList(GraphQLString) }),
nested: {
type: NestedType,
resolve: () => ({}),
},
nnList: fieldWithInputArg({
type: new GraphQLNonNull(new GraphQLList(GraphQLString)),
}),
Expand All @@ -149,7 +160,7 @@ function executeQuery(
query: string,
variableValues?: { [variable: string]: unknown },
) {
const document = parse(query);
const document = parse(query, { experimentalFragmentArguments: true });
return executeSync({ schema, document, variableValues });
}

Expand Down Expand Up @@ -1066,6 +1077,245 @@ describe('Execute: Handles inputs', () => {
});
});

describe('using fragment arguments', () => {
it('when there are no fragment arguments', () => {
const result = executeQuery(`
query {
...a
}
fragment a on TestType {
fieldWithNonNullableStringInput(input: "A")
}
`);
expect(result).to.deep.equal({
data: {
fieldWithNonNullableStringInput: '"A"',
},
});
});

it('when a value is required and provided', () => {
const result = executeQuery(`
query {
...a(value: "A")
}
fragment a($value: String!) on TestType {
fieldWithNonNullableStringInput(input: $value)
}
`);
expect(result).to.deep.equal({
data: {
fieldWithNonNullableStringInput: '"A"',
},
});
});

it('when a value is required and not provided', () => {
const result = executeQuery(`
query {
...a
}
fragment a($value: String!) on TestType {
fieldWithNullableStringInput(input: $value)
}
`);
expect(result).to.deep.equal({
data: {
fieldWithNullableStringInput: null,
},
});
});

it('when the definition has a default and is provided', () => {
const result = executeQuery(`
query {
...a(value: "A")
}
fragment a($value: String! = "B") on TestType {
fieldWithNonNullableStringInput(input: $value)
}
`);
expect(result).to.deep.equal({
data: {
fieldWithNonNullableStringInput: '"A"',
},
});
});

it('when the definition has a default and is not provided', () => {
const result = executeQuery(`
query {
...a
}
fragment a($value: String! = "B") on TestType {
fieldWithNonNullableStringInput(input: $value)
}
`);
expect(result).to.deep.equal({
data: {
fieldWithNonNullableStringInput: '"B"',
},
});
});

it('when the definition has a non-nullable default and is provided null', () => {
const result = executeQuery(`
query {
...a(value: null)
}
fragment a($value: String! = "B") on TestType {
fieldWithNullableStringInput(input: $value)
}
`);
expect(result).to.deep.equal({
data: {
fieldWithNullableStringInput: 'null',
},
});
});

it('when the definition has no default and is not provided', () => {
const result = executeQuery(`
query {
...a
}
fragment a($value: String) on TestType {
fieldWithNonNullableStringInputAndDefaultArgumentValue(input: $value)
}
`);
expect(result).to.deep.equal({
data: {
fieldWithNonNullableStringInputAndDefaultArgumentValue:
'"Hello World"',
},
});
});

it('when an argument is shadowed by an operation variable', () => {
const result = executeQuery(`
query($x: String! = "A") {
...a(x: "B")
}
fragment a($x: String) on TestType {
fieldWithNullableStringInput(input: $x)
}
`);
expect(result).to.deep.equal({
data: {
fieldWithNullableStringInput: '"B"',
},
});
});

it('when a nullable argument with a field default is not provided and shadowed by an operation variable', () => {
const result = executeQuery(`
query($x: String = "A") {
...a
}
fragment a($x: String) on TestType {
fieldWithNonNullableStringInputAndDefaultArgumentValue(input: $x)
}
`);
expect(result).to.deep.equal({
data: {
fieldWithNonNullableStringInputAndDefaultArgumentValue:
'"Hello World"',
},
});
});

it('when a fragment is used with different args', () => {
const result = executeQuery(`
query($x: String = "Hello") {
a: nested {
...a(x: "a")
}
b: nested {
...a(x: "b", b: true)
}
hello: nested {
...a(x: $x)
}
}
fragment a($x: String, $b: Boolean = false) on NestedType {
a: echo(input: $x) @skip(if: $b)
b: echo(input: $x) @include(if: $b)
}
`);
expect(result).to.deep.equal({
data: {
a: {
a: '"a"',
},
b: {
b: '"b"',
},
hello: {
a: '"Hello"',
},
},
});
});

it('when the argument variable is nested in a complex type', () => {
const result = executeQuery(`
query {
...a(value: "C")
}
fragment a($value: String) on TestType {
list(input: ["A", "B", $value, "D"])
}
`);
expect(result).to.deep.equal({
data: {
list: '["A", "B", "C", "D"]',
},
});
});

it('when argument variables are used recursively', () => {
const result = executeQuery(`
query {
...a(aValue: "C")
}
fragment a($aValue: String) on TestType {
...b(bValue: $aValue)
}
fragment b($bValue: String) on TestType {
list(input: ["A", "B", $bValue, "D"])
}
`);
expect(result).to.deep.equal({
data: {
list: '["A", "B", "C", "D"]',
},
});
});

it('when argument passed in as list', () => {
const result = executeQuery(`
query Q($opValue: String = "op") {
...a(aValue: "A")
}
fragment a($aValue: String, $bValue: String) on TestType {
...b(aValue: [$aValue, "B"], bValue: [$bValue, $opValue])
}
fragment b($aValue: [String], $bValue: [String], $cValue: String) on TestType {
aList: list(input: $aValue)
bList: list(input: $bValue)
cList: list(input: [$cValue])
}
`);
expect(result).to.deep.equal({
data: {
aList: '["A", "B"]',
bList: '[null, "op"]',
cList: '[null]',
},
});
});
});

describe('getVariableValues: limit maximum number of coercion errors', () => {
const doc = parse(`
query ($input: [String!]) {
Expand Down
28 changes: 17 additions & 11 deletions src/execution/collectFields.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,8 @@ import {
} from '../type/directives.js';
import type { GraphQLSchema } from '../type/schema.js';

import { keyForFragmentSpread } from '../utilities/keyForFragmentSpread.js';
import { substituteFragmentArguments } from '../utilities/substituteFragmentArguments.js';
import { typeFromAST } from '../utilities/typeFromAST.js';

import { getDirectiveValues } from './values.js';
Expand Down Expand Up @@ -124,7 +126,7 @@ function collectFieldsImpl(
selectionSet: SelectionSetNode,
fields: AccumulatorMap<string, FieldNode>,
patches: Array<PatchFields>,
visitedFragmentNames: Set<string>,
visitedFragmentKeys: Set<string>,
): void {
for (const selection of selectionSet.selections) {
switch (selection.kind) {
Expand Down Expand Up @@ -156,7 +158,7 @@ function collectFieldsImpl(
selection.selectionSet,
patchFields,
patches,
visitedFragmentNames,
visitedFragmentKeys,
);
patches.push({
label: defer.label,
Expand All @@ -172,24 +174,24 @@ function collectFieldsImpl(
selection.selectionSet,
fields,
patches,
visitedFragmentNames,
visitedFragmentKeys,
);
}
break;
}
case Kind.FRAGMENT_SPREAD: {
const fragName = selection.name.value;
const fragmentKey = keyForFragmentSpread(selection);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nitpick feedback: lets move this closer to where its used so the earlier short-circuits can be checked first.

Could this be a potential performance concern since collectFields is a highly called function? Is there any opportunity to compute these keys lazily - only doing so if there's first a simple name collision?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's likely worth testing the performance benchmarks by making this lazy, but my suspicion is it will be unlikely to make a difference in most cases and/or the complexity of keeping an additional layer of cache (today it's key => fragment, after it would be name => (first fragment, spreads-to-merge, computed-keys-if-multiple-spreads => fragment)).

Because we don't have a real scope, we do want to cache the spreads' fragment result, as we could hit ...profilePicture(size: 30) many times and we don't really want to visit profilePicture and replace $size each time.

Copy link
Contributor

@leebyron leebyron Feb 9, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+100 - I got too far into solutioning here and you are correct that we should try to measure some degenerate cases to understand it first. See the other thread about validation which might side-step all of this by removing the need for it in the first place


if (!shouldIncludeNode(variableValues, selection)) {
continue;
}

const defer = getDeferValues(operation, variableValues, selection);
if (visitedFragmentNames.has(fragName) && !defer) {
if (visitedFragmentKeys.has(fragmentKey) && !defer) {
continue;
}

const fragment = fragments[fragName];
const fragment = fragments[selection.name.value];
if (
!fragment ||
!doesFragmentConditionMatch(schema, fragment, runtimeType)
Expand All @@ -198,9 +200,13 @@ function collectFieldsImpl(
}

if (!defer) {
visitedFragmentNames.add(fragName);
visitedFragmentKeys.add(fragmentKey);
}

const fragmentSelectionSet = substituteFragmentArguments(
fragment,
selection,
);
if (defer) {
const patchFields = new AccumulatorMap<string, FieldNode>();
collectFieldsImpl(
Expand All @@ -209,10 +215,10 @@ function collectFieldsImpl(
variableValues,
operation,
runtimeType,
fragment.selectionSet,
fragmentSelectionSet,
patchFields,
patches,
visitedFragmentNames,
visitedFragmentKeys,
);
patches.push({
label: defer.label,
Expand All @@ -225,10 +231,10 @@ function collectFieldsImpl(
variableValues,
operation,
runtimeType,
fragment.selectionSet,
fragmentSelectionSet,
fields,
patches,
visitedFragmentNames,
visitedFragmentKeys,
);
}
break;
Expand Down
Loading