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

Add coerceInputLiteral() #3809

Merged
merged 1 commit into from
Sep 17, 2024

Conversation

yaacovCR
Copy link
Contributor

@yaacovCR yaacovCR commented Dec 31, 2022

#3092 rebased on main.

Depends on #3808

@leebyron comments from original PR:

Removes valueFromAST() and adds coerceInputLiteral() as an additional export from coerceInputValue.

The implementation is almost exactly the same as valueFromAST() with a slightly more strict type signature and refactored tests to improve coverage (the file unit test has 100% coverage)

While this does not change any behavior, it could be breaking if you rely directly on the valueFromAST() method. Use coerceInputLiteral() as a direct replacement.

Changes from the original:

  1. Deprecation rather than removal of valueFromAST().
  2. coerceInputLiteral() and only coerceInputLiteral() supports fragment variables in addition to operation variables.

@netlify
Copy link

netlify bot commented Dec 31, 2022

Deploy Preview for compassionate-pike-271cb3 ready!

Name Link
🔨 Latest commit ed149db
🔍 Latest deploy log https://app.netlify.com/sites/compassionate-pike-271cb3/deploys/66e96ab7c79e8700079b6a64
😎 Deploy Preview https://deploy-preview-3809--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.

@yaacovCR yaacovCR added the PR: breaking change 💥 implementation requires increase of "major" version number label Dec 31, 2022
@github-actions
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 yaacovCR changed the title Coerce input literal rebased Add coerceInputLiteral() Dec 31, 2022
@yaacovCR yaacovCR force-pushed the coerce-input-literal-rebased branch from 3f694fd to cdf3f24 Compare January 1, 2023 19:35
@yaacovCR yaacovCR force-pushed the coerce-input-literal-rebased branch 4 times, most recently from 96b2fcc to 2fc8629 Compare February 6, 2023 12:34
@yaacovCR yaacovCR force-pushed the coerce-input-literal-rebased branch 2 times, most recently from 319f4a7 to bf89146 Compare May 31, 2023 11:24
@yaacovCR yaacovCR force-pushed the coerce-input-literal-rebased branch from bf89146 to e520725 Compare March 20, 2024 11:00
@yaacovCR yaacovCR requested a review from a team as a code owner September 14, 2024 19:08
@yaacovCR yaacovCR force-pushed the coerce-input-literal-rebased branch 12 times, most recently from 320a105 to ec0ca36 Compare September 15, 2024 11:54
Removes `valueFromAST()` and adds `coerceInputLiteral()` as an additional export from `coerceInputValue`.

The implementation is almost exactly the same as `valueFromAST()` with a slightly more strict type signature and refactored tests to improve coverage (the file unit test has 100% coverage)

While this does not change any behavior, it could be breaking if you rely directly on the valueFromAST() method. Use `coerceInputLiteral()` as a direct replacement.
@yaacovCR yaacovCR added PR: feature 🚀 requires increase of "minor" version number and removed PR: breaking change 💥 implementation requires increase of "major" version number labels Sep 17, 2024
@yaacovCR yaacovCR merged commit 7875552 into graphql:main Sep 17, 2024
20 checks passed
@yaacovCR yaacovCR deleted the coerce-input-literal-rebased branch September 17, 2024 13:54
yaacovCR added a commit that referenced this pull request Sep 23, 2024
[#3074 rebased on
main](#3074).

Depends on #3809

@leebyron comments from original PR (edited, hopefully correctly):

> Fixes #3051
> 
> This change solves the problem of default values defined via SDL not
always resolving correctly through introspection by preserving the
original GraphQL literal in the schema definition. This changes argument
and input field definitions `defaultValue` field from just the "value"
to a new `GraphQLDefaultValueUsage` type which contains either (EDIT:
but not both!) "value" and "literal" fields.
> 
> Here is the flow for how a default value defined in an SDL would be
converted into a functional schema and back to an SDL:
> 
> **Before this change:**
> 
> ```
> (SDL) --parse-> (AST) --coerceInputLiteral--> (defaultValue config)
--valueToAST--> (AST) --print --> (SDL)
> ```
> 
> `coerceInputLiteral` performs coercion which is a one-way function,
and `valueToAST` is unsafe and set to be deprecated in #3049.
> 
> **After this change:**
> 
> ```
> (SDL) --parse-> (defaultValue literal config) --print --> (SDL)
> ```

Co-authored-by: Lee Byron <lee.byron@robinhood.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
PR: feature 🚀 requires increase of "minor" version number
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants