Skip to content

Allow scalar parse* and serialized methods to access context #3600

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

Closed
wants to merge 1 commit into from

Conversation

yaacovCR
Copy link
Contributor

@yaacovCR yaacovCR commented May 19, 2022

See #3539 for more details.

Even when provided, context must be optional because:

  1. Default values for scalar are parsed when building a schema, outside of the execution context.
  2. Default values for scalar are serialized during findBreakingChanges and other schema operations, also be outside of the execution context.

BREAKING CHANGES are included for argument lists of several functions to include the optional context parameter:

  1. getVariableValues
  2. getArgumentValues
  3. coerceInputValue
  4. valueFromAST
  5. astFromValue

@netlify
Copy link

netlify bot commented May 19, 2022

👷 Deploy request for compassionate-pike-271cb3 pending review.

Visit the deploys page to approve it

Name Link
🔨 Latest commit fe2a096

@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 added the PR: breaking change 💥 implementation requires increase of "major" version number label May 19, 2022
@yaacovCR yaacovCR self-assigned this May 19, 2022
@yaacovCR yaacovCR requested a review from a team May 19, 2022 23:55
@yaacovCR yaacovCR changed the title Allow scalar methods to access context Allow scalar parse* and serialized methods to access context May 20, 2022
Even when provided, `context` must be optional because:
1. Default values for scalar are parsed when building a schema, outside of the execution context.
2. Default values for scalar are serialized during findBreakingChanges and other schema operations, also be outside of the execution context.

BREAKING CHANGES are included for argument lists of several functions to include the optional context parameter:
1. `getVariableValues`
2. `getArgumentValues`
3. `coerceInputValue`
4. `valueFromAST`
5. `astFromValue`
@yaacovCR
Copy link
Contributor Author

Updated the PR with necessary changes to astFromValue.

Copy link
Member

@IvanGoncharov IvanGoncharov left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Strongly oppose this.
Scalars are used in many situations that don't have access to context and you made it optional for that reason. Scalars should behave in exact same way no matter where they are used.

Moreover, we deliberately do not pass context in functions like validate that was already discussed and rejected in another issue.
In use case outlined in #3539 that means if you specify scalar as literal you will get not localize error message (through validate) but if you pass it as the variable it became localized.

Same with other situations where scalars used outside of variables, e.g. default values in SDL you will get non-localized values.
I think localization usecase should be solved through error's extensions and formatError function from GraphQL server lib as outlined here: #3539

@yaacovCR
Copy link
Contributor Author

I don't think I am as dogmatic about scalars always behaving exactly--exactly--exactly the same in every context.

But, I do think that the solution that @IvanGoncharov has suggested works better in the proposed use case, and so this PR is currently a solution in search of a problem.... or.... more specifically, a use case that justifies the complexity and caveats.

Thanks for the review!

@yaacovCR yaacovCR closed this May 22, 2022
@yaacovCR yaacovCR deleted the context branch May 22, 2022 19:31
@yaacovCR yaacovCR restored the context branch May 22, 2022 19:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
PR: breaking change 💥 implementation requires increase of "major" version number
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants