Skip to content

Resolve missing types "Boolean", "String"#1355

Merged
trevor-scheer merged 4 commits intomasterfrom
trevor/fix-graphql-version
Jun 27, 2019
Merged

Resolve missing types "Boolean", "String"#1355
trevor-scheer merged 4 commits intomasterfrom
trevor/fix-graphql-version

Conversation

@trevor-scheer
Copy link
Copy Markdown
Contributor

@trevor-scheer trevor-scheer commented Jun 17, 2019

Preface

A change to graphql's buildClientSchema removed some behavior that we depended on. Previously, with caching in place, basic types (Boolean, String, ...) were returned even if they didn't exist in typeMap. The "fix" was itself a breaking change, adding basic types that weren't previously added to the typeMap.

Full discussion here: graphql/graphql-js@183ff32#r32971387

Chain of events

Approach / Discussion

@justinanastos and I discovered with some digging that we depended on buildClientSchema to fill in the missing built-in types for us (we were handing off an incomplete schema, but it would correct our error). We realized that we can request the built-in types and solve the problem on our end by passing a complete schema.

This PR addresses a couple issues that stem from the ongoing graphql issue (and subsequent fixes):

  1. We can request the built in types from Engine so they won't be missing to begin with
  2. Disallow versions of graphql that include the broken fix graphql@14.2.1 - 14.3.0.

Why exclude the range 14.2.1 - 14.3.0?

The broken fix causes a regression for service:push. Through the version range of the broken fix, ALL built in types are included during buildClientSchema (even if they're not part of the schema). This causes a hash mismatch between the pushed schema and the schema hash that Apollo Server recognizes.

Fixes #1296
Fixes #1295

TODO:

  • Update CHANGELOG.md* with your change (include reference to issue & this PR)
  • Make sure all of the significant new logic is covered by tests
  • Rebase your changes on master so that they can be merged easily
  • Make sure all tests and linter rules pass

*Make sure changelog entries note which project(s) has been affected. See older entries for examples on what this looks like.

A change to buildClientSchema removed some behavior that we
depended on. Previously, with caching in place, basic types
(Boolean, String, ...) were returned even if they didn't exist
in typeMap. The "fix" was itself a breaking change, adding basic
types that weren't previously added to the typeMap.

Full discussion here: graphql/graphql-js@183ff32#r32971387

This commit addresses a couple issues:
1) Request the built in types from Engine
2) Disallow versions of graphql that include the broken fix

The broken fix causes a regression for service:push. During the
broken fix (graphql@14.2.1-14.3.0), ALL built in types are included
during buildClientSchema (even if they're not part of the schema).
This causes a hash mismatch between the pushed schema and the
schema hash that Apollo Server recognizes.
@trevor-scheer trevor-scheer requested a review from abernix June 17, 2019 20:59
Copy link
Copy Markdown
Member

@abernix abernix left a comment

Choose a reason for hiding this comment

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

LGTM!

Copy link
Copy Markdown
Member

@abernix abernix left a comment

Choose a reason for hiding this comment

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

Note that, as we discovered on our out-of-band call, spaces must be used in the version ranges in order to properly differentiate them from just general hyphens in version names, as I've "suggested" below.

Comment thread packages/apollo/package.json Outdated
Copy link
Copy Markdown
Member

@abernix abernix left a comment

Choose a reason for hiding this comment

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

Same as previous comment!

Comment thread packages/apollo-language-server/package.json Outdated
Great catch @abernix, this fixes an issue with the version range
previously wasn't parsed correctly by semver.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2.12.3: Error: Cannot use GraphQLSchema "[object GraphQLSchema]" from another module or realm Could not fetch manifest (Access denied.)

2 participants