Skip to content

adjust GraphQLResolveInfo to remove breaking change from v16#4303

Closed
yaacovCR wants to merge 1 commit intographql:mainfrom
yaacovCR:ease-variable-values-upgrade-path
Closed

adjust GraphQLResolveInfo to remove breaking change from v16#4303
yaacovCR wants to merge 1 commit intographql:mainfrom
yaacovCR:ease-variable-values-upgrade-path

Conversation

@yaacovCR
Copy link
Copy Markdown
Contributor

@yaacovCR yaacovCR commented Nov 26, 2024

#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
adjust GraphQLResolveInfo to remove breaking change from v16
1d19845 
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
@netlify
Copy link
Copy Markdown

netlify Bot commented Nov 26, 2024 •
edited
Loading

✅ 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.

@github-actions
Copy link
Copy Markdown

github-actions Bot commented Nov 26, 2024

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
Copy Markdown
Contributor Author

yaacovCR commented Nov 26, 2024

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
Copy Markdown
Contributor Author

yaacovCR commented Nov 27, 2024

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

@yaacovCR
Copy link
Copy Markdown
Contributor Author

yaacovCR commented Nov 28, 2024

Closing for now!

@yaacovCR yaacovCR closed this Nov 28, 2024
@yaacovCR yaacovCR mentioned this pull request Nov 28, 2024
Merged
@yaacovCR yaacovCR deleted the ease-variable-values-upgrade-path branch December 16, 2024 19:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

PR: bug fix 🐞 requires increase of "patch" version number

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

1 participant

@yaacovCR