fix(replaceVariables): preserve sources for fragment variable values #4389
Conversation
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
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.
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.