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 valueToLiteral() #3812

Open
wants to merge 6 commits into
base: main
Choose a base branch
from

Conversation

yaacovCR
Copy link
Contributor

@yaacovCR yaacovCR commented Jan 1, 2023

#3065 rebased on main.

Depends on #3811

@leebyron comments from original PR:

Provides the "Value to Literal" methods in this data flow chart.

  • Adds valueToLiteral() which takes an external input value and translates it to a literal, allowing for custom scalars to define this behavior.

This also adds important changes to Input Coercion, especially for custom scalars:

  • The value provided to parseLiteral is now ConstValueNode and the second variables argument has been removed. For all built-in scalars this has no effect, but any custom scalars which use complex literals no longer need to do variable reconciliation manually (in fact most do not -- this has been an easy subtle bug to miss).
    This behavior is possible with the addition of replaceVariables

@yaacovCR yaacovCR added the PR: feature 🚀 requires increase of "minor" version number label Jan 1, 2023
@yaacovCR yaacovCR requested a review from leebyron January 1, 2023 14:00
@github-actions
Copy link

github-actions bot commented Jan 1, 2023

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

@netlify
Copy link

netlify bot commented Jan 1, 2023

Deploy Preview for compassionate-pike-271cb3 ready!

Name Link
🔨 Latest commit b8dec10
🔍 Latest deploy log https://app.netlify.com/sites/compassionate-pike-271cb3/deploys/65fac7dfc3da660008feaba9
😎 Deploy Preview https://deploy-preview-3812--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 force-pushed the value-to-literal-rebased branch 4 times, most recently from e740389 to f03f2c5 Compare January 1, 2023 20:06
@yaacovCR yaacovCR force-pushed the value-to-literal-rebased branch 7 times, most recently from 91cf2c1 to b3fbe95 Compare February 6, 2023 12:53
@yaacovCR yaacovCR force-pushed the value-to-literal-rebased branch 2 times, most recently from 7afbe9b to 3ea9f70 Compare May 31, 2023 11:51
Implements graphql/graphql-spec#794

Adds:

* DOT punctuator in lexer
* Improvements to lexer errors around misuse of `.`
* Minor improvement to parser core which simplified this addition
* `SchemaCoordinate` node and `isSchemaCoodinate()` predicate
* Support in `print()` and `visit()`
* Added function `parseSchemaCoordinate()` since it is a parser entry point.
* Added function `resolveSchemaCoordinate()` and `resolveASTSchemaCoordinate()` which implement the semantics (name mirrored from `buildASTSchema`) as well as the return type `ResolvedSchemaElement`
* Defines a `GraphQLSchemaElement` base class which defines a `.coordinate` property and `toString`/`toJSON` methods.
* Adds base class to types, fields, arguments, input fields, enum values, and directives.
* Uses this in validation error printing string templates.
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.
Fixes graphql#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 or both "value" and "literal" fields.

Since either of these fields may be set, new functions for resolving a value or literal from either have been added - `getLiteralDefaultValue` and `getCoercedDefaultValue` - these replace uses that either assumed a set value or were manually converting a value back to a literal.

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 graphql#3049.

**After this change:**

```
(SDL) --parse-> (defaultValue literal config) --print --> (SDL)
```
By way of introducing type `VariableValues`, allows `getVariableValues` to return both the coerced values as well as the original sources, which are then made available in `ExecutionContext`.
* Adds `valueToLiteral()` which takes an external value and translates it to a literal, allowing for custom scalars to define this behavior.

This also adds important changes to Input Coercion, especially for custom scalars:

* The value provided to `parseLiteral` is now `ConstValueNode` and the second `variables` argument has been removed. For all built-in scalars this has no effect, but any custom scalars which use complex literals no longer need to do variable reconciliation manually (in fact most do not -- this has been an easy subtle bug to miss).

  This behavior is possible with the addition of `replaceASTVariables`
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.

None yet

2 participants