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

Update to graphql-js 14.x #953

Merged
merged 19 commits into from
Sep 19, 2018
Merged

Update to graphql-js 14.x #953

merged 19 commits into from
Sep 19, 2018

Conversation

hwillson
Copy link
Contributor

@hwillson hwillson commented Sep 14, 2018

This PR updates the graphql-tools peerDependencies to allow graphql 14.x, and enforces graphql 14.x in its devDependencies. All changes have been made in a way that is backwards compatible with graphql 0.13.x, but anyone looking to upgrade to graphql 14.x should keep in mind that graphql 14.x introduced several breaking changes. Because of this we're likely going to major version bump graphql-tools for the next release.

Most of the changes in this PR are pretty straightforward, with one main exception. The current version of graphql-tools is relying on graphql 0.13 functionality to achieve internal enum value support. graphql 14.x has tightened up public facing API access, which means the current method of handling internal enum values no longer works with graphql 14.x. Because of this, we now have to transform existing enums into new enums that include an internal value. This means we're adding a new schema transformation step to schema's that include enums with enum resolvers.

Fixes #945.

Work remaining:

  • Update @types/graphql
  • Allow graphql 14.x in all peer projects and get new versions published (apollo-link, graphql-subscriptions)
  • Stop running CI on Node 4; Add in Node 10

@hwillson hwillson changed the title Update to graphql-js 14.x [WIP] Update to graphql-js 14.x Sep 14, 2018
@hwillson
Copy link
Contributor Author

Linking in the PR's that need to be addressed with this one:

apollographql/apollo-link#789
apollographql/graphql-subscriptions#171

@hwillson
Copy link
Contributor Author

Waiting on apollographql/graphql-subscriptions#172 before we can finalize this.

@hwillson hwillson changed the title [WIP] Update to graphql-js 14.x Update to graphql-js 14.x Sep 17, 2018
Copy link
Collaborator

@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.

Only one item which I think warrants tweaking, but otherwise, LGTM!

Nice work here!

@@ -51,6 +52,10 @@ function addResolveFunctionsToSchema(
? extendResolversFromInterfaces(schema, inputResolvers)
: inputResolvers;

// Used to map the external value of an enum to its internal value, when
// that internal value is provided by a resolver.
const enumValueMap = {};
Copy link
Collaborator

Choose a reason for hiding this comment

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

Rather than using the object literal notation, which inherits from Object.prototype, I believe we should be careful to use Object.create(null) here since the result of getType(...).name (a user-controlled string) could get set into it — potentially a problem with JavaScript internal properties like length, prototype, etc.

For example, without this (unless this is caught somewhere else that I'm missing?) weird behavior could ensue when the return value of getType(...).name is set into enumValueMap via enumValueMap[valueReturnedFromGetTypeName] =:

  const sdl = `
    enum length { LONG SHORT }
    type Query { long: length }
  `;

  require("graphql")
    .buildSchema(sdl)
    .getType("length").name; // "length"

(And similarly with say, constructor or prototype in place of length).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh yes, great point @abernix - I'll adjust, thanks!

@@ -88,9 +88,10 @@ export function recreateType(
const newValues = {};
values.forEach(value => {
newValues[value.name] = {
value: value.name,
value: value.value,
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can I inquire more details about this particular change (name to value)?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure thing! Normally when using a GraphQLEnumType object, the name and value properties of an enum value are the same. E.g. Color.RED would have RED stored as both it's name and value. graphql-tools internal enum value support however needs to store the new internal value we want to use into the new GraphQLEnumType value that's created. We have to keep the name of the value the same as it was before (e.g. name stays as RED), but here we're adjusting the value to be the new internal value that's coming from the enum resolver (that we setup in src/generate/addResolveFunctionsToSchema.ts).

We could have added some logic in here to only change the value to value.value when we have enum resolvers setup (in other words only when we want to use new internal enum values), but since value is always the same as name when using a normal enum without any internal values, this code change should work in both cases.

Enum with an external value only:
Color.RED --> name = RED, value = 'RED'

Enum with an external and internal value:
Color.RED --> name = RED, value = '#ff0000'

So using value.value in value in both cases above gives us what we need.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Excellent explanation. Thank you!

@hwillson hwillson mentioned this pull request Sep 19, 2018
5 tasks
@hwillson hwillson merged commit b85137b into master Sep 19, 2018
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.

Upgrade graphql peerDependency to 14.0.0
2 participants