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

Correct isSpecifiedScalarType refinement, fix #2153 #2194

Closed
wants to merge 2 commits into from

Conversation

tgriesser
Copy link
Contributor

@tgriesser tgriesser commented Sep 23, 2019

Fixes #2153

Ran into this when looking to upgrade to the latest in graphql-nexus. To reproduce:

if (isScalarType(type)) {
  if (isSpecifiedScalarType(type)) {
    rootTypeMap[type.name] = SpecifiedScalars[type.name]
  } else {
    rootTypeMap[type.name] = "any"
  }
}

Before the changes in this PR, type in the else block is inferred to be never, because isSpecifiedScalarType is currently narrows to all scalars rather than just the "specified" ones, and as such type.name is invalid:

Screen Shot 2019-09-22 at 11 21 55 PM

By adding the & { name: '...' } for each of the specified scalars, we give enough context to know that the else block is a valid codepath for all "non-specified" scalars.

@IvanGoncharov
Copy link
Member

Had a deep and long conversation with @tgriesser that helped me to formulate my position so to summaries it here:

  • It's typescript specific refinement and it's not about syntax, entire idea of refining possible value of string to Int | Float | ... is TS specific. Which is problematic for graphql-js goal of being a reference implementation that should be easily portable into other languages.
  • It gives a false sense of security: if we add new scalar (e.g. GraphQLDateTime discussed in [RFC] New Scalar for date-time graphql-spec#579) TS code that was validated against older versions will fail even through adding new standard scalar is a non-breaking change.

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.

TS: isSpecifiedScalarType incorrectly refines type
2 participants