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.

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

adjust GraphQLResolveInfo to remove breaking change from v16 #4303

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

yaacovCR
Copy link
Contributor

#3811 internally preserved the sources of variable values so that we could properly replace variables within complex scalars, but it also exposed this within the GraphQLResolveInfo interface available to resolvers in a breaking manner, by changing the type of the variableValues property.

This PR reverts the change to the variableValues property, but exposed the extended variable information under a new variableValuesWithSources property.

graphql#3811 internally preserved the sources of variable values so that we could properly replace variables within complex scalars, but it also exposed this within the GraphQLResolveInfo interface available to resolvers in a breaking manner, by changing the type of the `variableValues` property.

This PR reverts the change to the `variableValues` property, but exposed the extended variable information under a new `variableValuesWithSources` property.
@yaacovCR yaacovCR requested a review from a team as a code owner November 26, 2024 21:30
@yaacovCR yaacovCR added the PR: bug fix 🐞 requires increase of "patch" version number label Nov 26, 2024
Copy link

netlify bot commented Nov 26, 2024

Deploy Preview for compassionate-pike-271cb3 ready!

Name Link
🔨 Latest commit 1d19845
🔍 Latest deploy log https://app.netlify.com/sites/compassionate-pike-271cb3/deploys/67463de51b56a30008f8599b
😎 Deploy Preview https://deploy-preview-4303--compassionate-pike-271cb3.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

Copy link

Hi @yaacovCR, I'm @github-actions bot happy to help you with this PR 👋

Supported commands

Please post this commands in separate comments and only one per comment:

  • @github-actions run-benchmark - Run benchmark comparing base and merge commits for this PR
  • @github-actions publish-pr-on-npm - Build package from this PR and publish it on NPM

@yaacovCR
Copy link
Contributor Author

Not 100% sure about this:

from the what's new in v17 WIP at #4198:

The return type of getVariableValues() has been updated, as have the provided arguments to getArgumentValues(), coerceInputLiteral(), and collectFields(), and getDirectiveValues().

I forgot to write there explicitly that we changed the type of variableValues, although it's partly what I meant when I wrote "throughout the code base." I guess I'm wondering if we need to change it consistently throughout the code base. Those doing lookahead using GraphQLResolveInfo will probably need the variable sources for the same reason we do (to replace variable values safely within complex scalar literals), but they can access that in a new properly.

As of now, collectFields() doesn't seem to require the variable sources, we could probably just pass the coerced variables, but because the functions it depends on are more generic, they now take the full VariableValues with the sources.

Is there a way to make this even less breaking? Should we skip this PR and just be 100% consistent. Questions!

@yaacovCR yaacovCR marked this pull request as draft November 27, 2024 12:41
@yaacovCR
Copy link
Contributor Author

I'm favoring today just leaving things as is for consistency, feedback welcome!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
PR: bug fix 🐞 requires increase of "patch" version number
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant