fix(replaceVariables): preserve sources for fragment variable values#4389
Merged
yaacovCR merged 6 commits intographql:mainfrom May 12, 2025
Merged
fix(replaceVariables): preserve sources for fragment variable values#4389yaacovCR merged 6 commits intographql:mainfrom
yaacovCR merged 6 commits intographql:mainfrom
Conversation
yaacovCR
force-pushed
the
fragment-variables
branch
from
e6fa60a to
3f6e32b
Compare
non-specified behavior but supported through v17 (with operation variables)
yaacovCR
force-pushed
the
fragment-variables
branch
from
44d33e4 to
32541f6
Compare
Contributor
Author
|
I broke this up into separate commits to aid review. First commit: introduces the failing test. |
by introducing internal helper coerceArgument goals: (1) removes need to build temporary array of `varSignatures` in `getFragmentVariableValues()` (2) removes need to rebuild map of `varSignatures` when `getFragmentVariableValues()` would call `experimentalGetArgumentValues()`. (3) reuses `argumentNode` variable instead of duplicating selection from the `argNodeMap` (4) allows us to preserve original `getArgumentValues()` signature and dispense with `experimentalGetArgumentValues()` altogether.
yaacovCR
changed the title
yaacovCR
added a commit
to yaacovCR/transform
that referenced
this pull request
May 6, 2025
for now we will disable support for fragment arguments, pending graphql/graphql-js#4389
yaacovCR
added a commit
to yaacovCR/transform
that referenced
this pull request
May 7, 2025
* minimize TransformationContext * consolidate loops * switch from filtering to actual routing of root fields * for now we will disable support for fragment arguments, pending graphql/graphql-js#4389 * add changeset * fix support for fragment argument * lint
JoviDeCroock
approved these changes
May 12, 2025
yaacovCR
added a commit
to yaacovCR/graphql-js
that referenced
this pull request
Dec 17, 2025
…raphql#4389) The new flow for coercing input literals -- see graphql#3814 -- introduces a `replaceVariables()` prior to calling `coerceInputLiteral()` such that we go: from `ValueNode` => `ConstValueNode` => coerced value The main advantages being that: (1) we can trivially support embedding variables within complex scalars -- despite this being non-spec defined behavior! (2) userland implementations of custom scalar `coerceInputLiteral()` methods do not need to worry about handling variables or fragment variables. Prior to this PR, we did not properly handle this in the case of fragment definition variables/fragment spread arguments. `replaceVariableValues()` assumes that the uncoerced value is preserved as source, but instead of grabbing this value from the argument on the spread, we were incorrectly attempting to retrieve the already stored value from existing variables. This was not caught because we previously did not have any actual tests for this custom unspecified behavior and were using incorrect types and bespoke builders in our tests for `replaceVariables()`. This PR: (a) fixes the bug by storing the argument from the spread (b) fixes our bespoke builders in `replaceVariables()` (c) add explicit tests for embedding variables within custom scalars to protect against future changes. As a post-script, because within `getFragmentVariableValues()` we now end up within a `ValueNode` stored on source, to coerce this value, we can extract a new helper `coerceArgumentValue()` from `experimentalGetArgumentValues()`, rather than calling it after we are done for all the values, which has some additional overhead. This has the side benefit of removing the need for a separate `experimentalGetArgumentValues()` function altogether. We initially introduced it to have a more flexible signature for `getArgumentValues()` that encompasses argument for fragment spreads, but now this is taken care of using our more directed helper.
yaacovCR
added a commit
to yaacovCR/graphql-js
that referenced
this pull request
Dec 17, 2025
…raphql#4389) The new flow for coercing input literals -- see graphql#3814 -- introduces a `replaceVariables()` prior to calling `coerceInputLiteral()` such that we go: from `ValueNode` => `ConstValueNode` => coerced value The main advantages being that: (1) we can trivially support embedding variables within complex scalars -- despite this being non-spec defined behavior! (2) userland implementations of custom scalar `coerceInputLiteral()` methods do not need to worry about handling variables or fragment variables. Prior to this PR, we did not properly handle this in the case of fragment definition variables/fragment spread arguments. `replaceVariableValues()` assumes that the uncoerced value is preserved as source, but instead of grabbing this value from the argument on the spread, we were incorrectly attempting to retrieve the already stored value from existing variables. This was not caught because we previously did not have any actual tests for this custom unspecified behavior and were using incorrect types and bespoke builders in our tests for `replaceVariables()`. This PR: (a) fixes the bug by storing the argument from the spread (b) fixes our bespoke builders in `replaceVariables()` (c) add explicit tests for embedding variables within custom scalars to protect against future changes. As a post-script, because within `getFragmentVariableValues()` we now end up within a `ValueNode` stored on source, to coerce this value, we can extract a new helper `coerceArgumentValue()` from `experimentalGetArgumentValues()`, rather than calling it after we are done for all the values, which has some additional overhead. This has the side benefit of removing the need for a separate `experimentalGetArgumentValues()` function altogether. We initially introduced it to have a more flexible signature for `getArgumentValues()` that encompasses argument for fragment spreads, but now this is taken care of using our more directed helper.
yaacovCR
added a commit
to yaacovCR/graphql-js
that referenced
this pull request
Dec 17, 2025
…raphql#4389) The new flow for coercing input literals -- see graphql#3814 -- introduces a `replaceVariables()` prior to calling `coerceInputLiteral()` such that we go: from `ValueNode` => `ConstValueNode` => coerced value The main advantages being that: (1) we can trivially support embedding variables within complex scalars -- despite this being non-spec defined behavior! (2) userland implementations of custom scalar `coerceInputLiteral()` methods do not need to worry about handling variables or fragment variables. Prior to this PR, we did not properly handle this in the case of fragment definition variables/fragment spread arguments. `replaceVariableValues()` assumes that the uncoerced value is preserved as source, but instead of grabbing this value from the argument on the spread, we were incorrectly attempting to retrieve the already stored value from existing variables. This was not caught because we previously did not have any actual tests for this custom unspecified behavior and were using incorrect types and bespoke builders in our tests for `replaceVariables()`. This PR: (a) fixes the bug by storing the argument from the spread (b) fixes our bespoke builders in `replaceVariables()` (c) add explicit tests for embedding variables within custom scalars to protect against future changes. As a post-script, because within `getFragmentVariableValues()` we now end up within a `ValueNode` stored on source, to coerce this value, we can extract a new helper `coerceArgumentValue()` from `experimentalGetArgumentValues()`, rather than calling it after we are done for all the values, which has some additional overhead. This has the side benefit of removing the need for a separate `experimentalGetArgumentValues()` function altogether. We initially introduced it to have a more flexible signature for `getArgumentValues()` that encompasses argument for fragment spreads, but now this is taken care of using our more directed helper.
yaacovCR
added a commit
to yaacovCR/graphql-js
that referenced
this pull request
Dec 18, 2025
…raphql#4389) The new flow for coercing input literals -- see graphql#3814 -- introduces a `replaceVariables()` prior to calling `coerceInputLiteral()` such that we go: from `ValueNode` => `ConstValueNode` => coerced value The main advantages being that: (1) we can trivially support embedding variables within complex scalars -- despite this being non-spec defined behavior! (2) userland implementations of custom scalar `coerceInputLiteral()` methods do not need to worry about handling variables or fragment variables. Prior to this PR, we did not properly handle this in the case of fragment definition variables/fragment spread arguments. `replaceVariableValues()` assumes that the uncoerced value is preserved as source, but instead of grabbing this value from the argument on the spread, we were incorrectly attempting to retrieve the already stored value from existing variables. This was not caught because we previously did not have any actual tests for this custom unspecified behavior and were using incorrect types and bespoke builders in our tests for `replaceVariables()`. This PR: (a) fixes the bug by storing the argument from the spread (b) fixes our bespoke builders in `replaceVariables()` (c) add explicit tests for embedding variables within custom scalars to protect against future changes. As a post-script, because within `getFragmentVariableValues()` we now end up within a `ValueNode` stored on source, to coerce this value, we can extract a new helper `coerceArgumentValue()` from `experimentalGetArgumentValues()`, rather than calling it after we are done for all the values, which has some additional overhead. This has the side benefit of removing the need for a separate `experimentalGetArgumentValues()` function altogether. We initially introduced it to have a more flexible signature for `getArgumentValues()` that encompasses argument for fragment spreads, but now this is taken care of using our more directed helper.
yaacovCR
added a commit
to yaacovCR/graphql-js
that referenced
this pull request
Dec 19, 2025
…raphql#4389) The new flow for coercing input literals -- see graphql#3814 -- introduces a `replaceVariables()` prior to calling `coerceInputLiteral()` such that we go: from `ValueNode` => `ConstValueNode` => coerced value The main advantages being that: (1) we can trivially support embedding variables within complex scalars -- despite this being non-spec defined behavior! (2) userland implementations of custom scalar `coerceInputLiteral()` methods do not need to worry about handling variables or fragment variables. Prior to this PR, we did not properly handle this in the case of fragment definition variables/fragment spread arguments. `replaceVariableValues()` assumes that the uncoerced value is preserved as source, but instead of grabbing this value from the argument on the spread, we were incorrectly attempting to retrieve the already stored value from existing variables. This was not caught because we previously did not have any actual tests for this custom unspecified behavior and were using incorrect types and bespoke builders in our tests for `replaceVariables()`. This PR: (a) fixes the bug by storing the argument from the spread (b) fixes our bespoke builders in `replaceVariables()` (c) add explicit tests for embedding variables within custom scalars to protect against future changes. As a post-script, because within `getFragmentVariableValues()` we now end up within a `ValueNode` stored on source, to coerce this value, we can extract a new helper `coerceArgumentValue()` from `experimentalGetArgumentValues()`, rather than calling it after we are done for all the values, which has some additional overhead. This has the side benefit of removing the need for a separate `experimentalGetArgumentValues()` function altogether. We initially introduced it to have a more flexible signature for `getArgumentValues()` that encompasses argument for fragment spreads, but now this is taken care of using our more directed helper.
yaacovCR
added a commit
to yaacovCR/graphql-js
that referenced
this pull request
Dec 22, 2025
…raphql#4389) The new flow for coercing input literals -- see graphql#3814 -- introduces a `replaceVariables()` prior to calling `coerceInputLiteral()` such that we go: from `ValueNode` => `ConstValueNode` => coerced value The main advantages being that: (1) we can trivially support embedding variables within complex scalars -- despite this being non-spec defined behavior! (2) userland implementations of custom scalar `coerceInputLiteral()` methods do not need to worry about handling variables or fragment variables. Prior to this PR, we did not properly handle this in the case of fragment definition variables/fragment spread arguments. `replaceVariableValues()` assumes that the uncoerced value is preserved as source, but instead of grabbing this value from the argument on the spread, we were incorrectly attempting to retrieve the already stored value from existing variables. This was not caught because we previously did not have any actual tests for this custom unspecified behavior and were using incorrect types and bespoke builders in our tests for `replaceVariables()`. This PR: (a) fixes the bug by storing the argument from the spread (b) fixes our bespoke builders in `replaceVariables()` (c) add explicit tests for embedding variables within custom scalars to protect against future changes. As a post-script, because within `getFragmentVariableValues()` we now end up within a `ValueNode` stored on source, to coerce this value, we can extract a new helper `coerceArgumentValue()` from `experimentalGetArgumentValues()`, rather than calling it after we are done for all the values, which has some additional overhead. This has the side benefit of removing the need for a separate `experimentalGetArgumentValues()` function altogether. We initially introduced it to have a more flexible signature for `getArgumentValues()` that encompasses argument for fragment spreads, but now this is taken care of using our more directed helper.
yaacovCR
added a commit
that referenced
this pull request
Dec 22, 2025
…4389) The new flow for coercing input literals -- see #3814 -- introduces a `replaceVariables()` prior to calling `coerceInputLiteral()` such that we go: from `ValueNode` => `ConstValueNode` => coerced value The main advantages being that: (1) we can trivially support embedding variables within complex scalars -- despite this being non-spec defined behavior! (2) userland implementations of custom scalar `coerceInputLiteral()` methods do not need to worry about handling variables or fragment variables. Prior to this PR, we did not properly handle this in the case of fragment definition variables/fragment spread arguments. `replaceVariableValues()` assumes that the uncoerced value is preserved as source, but instead of grabbing this value from the argument on the spread, we were incorrectly attempting to retrieve the already stored value from existing variables. This was not caught because we previously did not have any actual tests for this custom unspecified behavior and were using incorrect types and bespoke builders in our tests for `replaceVariables()`. This PR: (a) fixes the bug by storing the argument from the spread (b) fixes our bespoke builders in `replaceVariables()` (c) add explicit tests for embedding variables within custom scalars to protect against future changes. As a post-script, because within `getFragmentVariableValues()` we now end up within a `ValueNode` stored on source, to coerce this value, we can extract a new helper `coerceArgumentValue()` from `experimentalGetArgumentValues()`, rather than calling it after we are done for all the values, which has some additional overhead. This has the side benefit of removing the need for a separate `experimentalGetArgumentValues()` function altogether. We initially introduced it to have a more flexible signature for `getArgumentValues()` that encompasses argument for fragment spreads, but now this is taken care of using our more directed helper.
yaacovCR
added a commit
that referenced
this pull request
Dec 22, 2025
…4389) The new flow for coercing input literals -- see #3814 -- introduces a `replaceVariables()` prior to calling `coerceInputLiteral()` such that we go: from `ValueNode` => `ConstValueNode` => coerced value The main advantages being that: (1) we can trivially support embedding variables within complex scalars -- despite this being non-spec defined behavior! (2) userland implementations of custom scalar `coerceInputLiteral()` methods do not need to worry about handling variables or fragment variables. Prior to this PR, we did not properly handle this in the case of fragment definition variables/fragment spread arguments. `replaceVariableValues()` assumes that the uncoerced value is preserved as source, but instead of grabbing this value from the argument on the spread, we were incorrectly attempting to retrieve the already stored value from existing variables. This was not caught because we previously did not have any actual tests for this custom unspecified behavior and were using incorrect types and bespoke builders in our tests for `replaceVariables()`. This PR: (a) fixes the bug by storing the argument from the spread (b) fixes our bespoke builders in `replaceVariables()` (c) add explicit tests for embedding variables within custom scalars to protect against future changes. As a post-script, because within `getFragmentVariableValues()` we now end up within a `ValueNode` stored on source, to coerce this value, we can extract a new helper `coerceArgumentValue()` from `experimentalGetArgumentValues()`, rather than calling it after we are done for all the values, which has some additional overhead. This has the side benefit of removing the need for a separate `experimentalGetArgumentValues()` function altogether. We initially introduced it to have a more flexible signature for `getArgumentValues()` that encompasses argument for fragment spreads, but now this is taken care of using our more directed helper.
yaacovCR
added a commit
to yaacovCR/graphql-js
that referenced
this pull request
Jan 27, 2026
…raphql#4389) The new flow for coercing input literals -- see graphql#3814 -- introduces a `replaceVariables()` prior to calling `coerceInputLiteral()` such that we go: from `ValueNode` => `ConstValueNode` => coerced value The main advantages being that: (1) we can trivially support embedding variables within complex scalars -- despite this being non-spec defined behavior! (2) userland implementations of custom scalar `coerceInputLiteral()` methods do not need to worry about handling variables or fragment variables. Prior to this PR, we did not properly handle this in the case of fragment definition variables/fragment spread arguments. `replaceVariableValues()` assumes that the uncoerced value is preserved as source, but instead of grabbing this value from the argument on the spread, we were incorrectly attempting to retrieve the already stored value from existing variables. This was not caught because we previously did not have any actual tests for this custom unspecified behavior and were using incorrect types and bespoke builders in our tests for `replaceVariables()`. This PR: (a) fixes the bug by storing the argument from the spread (b) fixes our bespoke builders in `replaceVariables()` (c) add explicit tests for embedding variables within custom scalars to protect against future changes. As a post-script, because within `getFragmentVariableValues()` we now end up within a `ValueNode` stored on source, to coerce this value, we can extract a new helper `coerceArgumentValue()` from `experimentalGetArgumentValues()`, rather than calling it after we are done for all the values, which has some additional overhead. This has the side benefit of removing the need for a separate `experimentalGetArgumentValues()` function altogether. We initially introduced it to have a more flexible signature for `getArgumentValues()` that encompasses argument for fragment spreads, but now this is taken care of using our more directed helper.
yaacovCR
added a commit
that referenced
this pull request
Feb 22, 2026
…4389) The new flow for coercing input literals -- see #3814 -- introduces a `replaceVariables()` prior to calling `coerceInputLiteral()` such that we go: from `ValueNode` => `ConstValueNode` => coerced value The main advantages being that: (1) we can trivially support embedding variables within complex scalars -- despite this being non-spec defined behavior! (2) userland implementations of custom scalar `coerceInputLiteral()` methods do not need to worry about handling variables or fragment variables. Prior to this PR, we did not properly handle this in the case of fragment definition variables/fragment spread arguments. `replaceVariableValues()` assumes that the uncoerced value is preserved as source, but instead of grabbing this value from the argument on the spread, we were incorrectly attempting to retrieve the already stored value from existing variables. This was not caught because we previously did not have any actual tests for this custom unspecified behavior and were using incorrect types and bespoke builders in our tests for `replaceVariables()`. This PR: (a) fixes the bug by storing the argument from the spread (b) fixes our bespoke builders in `replaceVariables()` (c) add explicit tests for embedding variables within custom scalars to protect against future changes. As a post-script, because within `getFragmentVariableValues()` we now end up within a `ValueNode` stored on source, to coerce this value, we can extract a new helper `coerceArgumentValue()` from `experimentalGetArgumentValues()`, rather than calling it after we are done for all the values, which has some additional overhead. This has the side benefit of removing the need for a separate `experimentalGetArgumentValues()` function altogether. We initially introduced it to have a more flexible signature for `getArgumentValues()` that encompasses argument for fragment spreads, but now this is taken care of using our more directed helper.
yaacovCR
added a commit
that referenced
this pull request
Feb 23, 2026
…4389) The new flow for coercing input literals -- see #3814 -- introduces a `replaceVariables()` prior to calling `coerceInputLiteral()` such that we go: from `ValueNode` => `ConstValueNode` => coerced value The main advantages being that: (1) we can trivially support embedding variables within complex scalars -- despite this being non-spec defined behavior! (2) userland implementations of custom scalar `coerceInputLiteral()` methods do not need to worry about handling variables or fragment variables. Prior to this PR, we did not properly handle this in the case of fragment definition variables/fragment spread arguments. `replaceVariableValues()` assumes that the uncoerced value is preserved as source, but instead of grabbing this value from the argument on the spread, we were incorrectly attempting to retrieve the already stored value from existing variables. This was not caught because we previously did not have any actual tests for this custom unspecified behavior and were using incorrect types and bespoke builders in our tests for `replaceVariables()`. This PR: (a) fixes the bug by storing the argument from the spread (b) fixes our bespoke builders in `replaceVariables()` (c) add explicit tests for embedding variables within custom scalars to protect against future changes. As a post-script, because within `getFragmentVariableValues()` we now end up within a `ValueNode` stored on source, to coerce this value, we can extract a new helper `coerceArgumentValue()` from `experimentalGetArgumentValues()`, rather than calling it after we are done for all the values, which has some additional overhead. This has the side benefit of removing the need for a separate `experimentalGetArgumentValues()` function altogether. We initially introduced it to have a more flexible signature for `getArgumentValues()` that encompasses argument for fragment spreads, but now this is taken care of using our more directed helper.
yaacovCR
added a commit
that referenced
this pull request
Feb 24, 2026
…4389) The new flow for coercing input literals -- see #3814 -- introduces a `replaceVariables()` prior to calling `coerceInputLiteral()` such that we go: from `ValueNode` => `ConstValueNode` => coerced value The main advantages being that: (1) we can trivially support embedding variables within complex scalars -- despite this being non-spec defined behavior! (2) userland implementations of custom scalar `coerceInputLiteral()` methods do not need to worry about handling variables or fragment variables. Prior to this PR, we did not properly handle this in the case of fragment definition variables/fragment spread arguments. `replaceVariableValues()` assumes that the uncoerced value is preserved as source, but instead of grabbing this value from the argument on the spread, we were incorrectly attempting to retrieve the already stored value from existing variables. This was not caught because we previously did not have any actual tests for this custom unspecified behavior and were using incorrect types and bespoke builders in our tests for `replaceVariables()`. This PR: (a) fixes the bug by storing the argument from the spread (b) fixes our bespoke builders in `replaceVariables()` (c) add explicit tests for embedding variables within custom scalars to protect against future changes. As a post-script, because within `getFragmentVariableValues()` we now end up within a `ValueNode` stored on source, to coerce this value, we can extract a new helper `coerceArgumentValue()` from `experimentalGetArgumentValues()`, rather than calling it after we are done for all the values, which has some additional overhead. This has the side benefit of removing the need for a separate `experimentalGetArgumentValues()` function altogether. We initially introduced it to have a more flexible signature for `getArgumentValues()` that encompasses argument for fragment spreads, but now this is taken care of using our more directed helper.
yaacovCR
added a commit
that referenced
this pull request
Feb 24, 2026
…4389) The new flow for coercing input literals -- see #3814 -- introduces a `replaceVariables()` prior to calling `coerceInputLiteral()` such that we go: from `ValueNode` => `ConstValueNode` => coerced value The main advantages being that: (1) we can trivially support embedding variables within complex scalars -- despite this being non-spec defined behavior! (2) userland implementations of custom scalar `coerceInputLiteral()` methods do not need to worry about handling variables or fragment variables. Prior to this PR, we did not properly handle this in the case of fragment definition variables/fragment spread arguments. `replaceVariableValues()` assumes that the uncoerced value is preserved as source, but instead of grabbing this value from the argument on the spread, we were incorrectly attempting to retrieve the already stored value from existing variables. This was not caught because we previously did not have any actual tests for this custom unspecified behavior and were using incorrect types and bespoke builders in our tests for `replaceVariables()`. This PR: (a) fixes the bug by storing the argument from the spread (b) fixes our bespoke builders in `replaceVariables()` (c) add explicit tests for embedding variables within custom scalars to protect against future changes. As a post-script, because within `getFragmentVariableValues()` we now end up within a `ValueNode` stored on source, to coerce this value, we can extract a new helper `coerceArgumentValue()` from `experimentalGetArgumentValues()`, rather than calling it after we are done for all the values, which has some additional overhead. This has the side benefit of removing the need for a separate `experimentalGetArgumentValues()` function altogether. We initially introduced it to have a more flexible signature for `getArgumentValues()` that encompasses argument for fragment spreads, but now this is taken care of using our more directed helper.
yaacovCR
added a commit
that referenced
this pull request
Feb 24, 2026
…4389) The new flow for coercing input literals -- see #3814 -- introduces a `replaceVariables()` prior to calling `coerceInputLiteral()` such that we go: from `ValueNode` => `ConstValueNode` => coerced value The main advantages being that: (1) we can trivially support embedding variables within complex scalars -- despite this being non-spec defined behavior! (2) userland implementations of custom scalar `coerceInputLiteral()` methods do not need to worry about handling variables or fragment variables. Prior to this PR, we did not properly handle this in the case of fragment definition variables/fragment spread arguments. `replaceVariableValues()` assumes that the uncoerced value is preserved as source, but instead of grabbing this value from the argument on the spread, we were incorrectly attempting to retrieve the already stored value from existing variables. This was not caught because we previously did not have any actual tests for this custom unspecified behavior and were using incorrect types and bespoke builders in our tests for `replaceVariables()`. This PR: (a) fixes the bug by storing the argument from the spread (b) fixes our bespoke builders in `replaceVariables()` (c) add explicit tests for embedding variables within custom scalars to protect against future changes. As a post-script, because within `getFragmentVariableValues()` we now end up within a `ValueNode` stored on source, to coerce this value, we can extract a new helper `coerceArgumentValue()` from `experimentalGetArgumentValues()`, rather than calling it after we are done for all the values, which has some additional overhead. This has the side benefit of removing the need for a separate `experimentalGetArgumentValues()` function altogether. We initially introduced it to have a more flexible signature for `getArgumentValues()` that encompasses argument for fragment spreads, but now this is taken care of using our more directed helper.
yaacovCR
added a commit
that referenced
this pull request
Feb 24, 2026
…4389) The new flow for coercing input literals -- see #3814 -- introduces a `replaceVariables()` prior to calling `coerceInputLiteral()` such that we go: from `ValueNode` => `ConstValueNode` => coerced value The main advantages being that: (1) we can trivially support embedding variables within complex scalars -- despite this being non-spec defined behavior! (2) userland implementations of custom scalar `coerceInputLiteral()` methods do not need to worry about handling variables or fragment variables. Prior to this PR, we did not properly handle this in the case of fragment definition variables/fragment spread arguments. `replaceVariableValues()` assumes that the uncoerced value is preserved as source, but instead of grabbing this value from the argument on the spread, we were incorrectly attempting to retrieve the already stored value from existing variables. This was not caught because we previously did not have any actual tests for this custom unspecified behavior and were using incorrect types and bespoke builders in our tests for `replaceVariables()`. This PR: (a) fixes the bug by storing the argument from the spread (b) fixes our bespoke builders in `replaceVariables()` (c) add explicit tests for embedding variables within custom scalars to protect against future changes. As a post-script, because within `getFragmentVariableValues()` we now end up within a `ValueNode` stored on source, to coerce this value, we can extract a new helper `coerceArgumentValue()` from `experimentalGetArgumentValues()`, rather than calling it after we are done for all the values, which has some additional overhead. This has the side benefit of removing the need for a separate `experimentalGetArgumentValues()` function altogether. We initially introduced it to have a more flexible signature for `getArgumentValues()` that encompasses argument for fragment spreads, but now this is taken care of using our more directed helper.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The new flow for coercing input literals -- see #3814 -- introduces a
replaceVariables()prior to callingcoerceInputLiteral()such that we go:from
ValueNode=>ConstValueNode=> coerced valueThe main advantages being that:
(1) we can trivially support embedding variables within complex scalars -- despite this being non-spec defined behavior!
(2) userland implementations of custom scalar
coerceInputLiteral()methods do not need to worry about handling variables or fragment variables.Prior to this PR, we did not properly handle this in the case of fragment definition variables/fragment spread arguments.
replaceVariableValues()assumes that the uncoerced value is preserved as source, but instead of grabbing this value from the argument on the spread, we were incorrectly attempting to retrieve the already stored value from existing variables.This was not caught because we previously did not have any actual tests for this custom unspecified behavior and were using incorrect types and bespoke builders in our tests for
replaceVariables().This PR:
(a) fixes the bug by storing the argument from the spread
(b) fixes our bespoke builders in
replaceVariables()(c) add explicit tests for embedding variables within custom scalars to protect against future changes.
As a post-script, because within
getFragmentVariableValues()we now end up within aValueNodestored on source, to coerce this value, we can extract a new helpercoerceArgumentValue()fromexperimentalGetArgumentValues(), rather than calling it after we are done for all the values, which has some additional overhead.This has the side benefit of removing the need for a separate
experimentalGetArgumentValues()function altogether. We initially introduced it to have a more flexible signature forgetArgumentValues()that encompasses argument for fragment spreads, but now this is taken care of using our more directed helper.