Upgrade graphql peerDependency to 14.0.0

[jonaskello]

The graphql package is released in major version at 14.0.0 and it would be nice if it could be supported as peerDependency.

[IvanGoncharov]

Just a note from graphql core contributor, 14.0.0 contains a few breaking changes:
https://github.com/graphql/graphql-js/releases/tag/v14.0.0
And most importantly it requires you to explicitly define directives (including its argument and locations) before or together with your SDL.
Also, note that some functions became deprecated and would be removed in 15.0.0.

Please free to ping me if you need any help with migration.

[hwillson]

Hi @IvanGoncharov - thanks for your offer to help. I'm working on updating graphql-tools to use graphql 14.x, and have hit a bit of a roadblock.

I see that in graphql 14.x, the GraphQLEnumType class received some performance updates (graphql/graphql-js@433774d). Due to these updates, there is no longer a way for us to override values returned by the GraphQLEnumType serialize method (https://github.com/graphql/graphql-js/blob/3595ea922613f3e13a07185407dd70ed45de7a66/src/type/definition.js#L1083). This means graphql 14.x breaks our ability to support adding internal values for enums (https://www.apollographql.com/docs/graphql-tools/scalars.html#internal-values).

In graphql 0.13.x and earlier, the GraphQLEnumType class was setup such that we could get in and override an enum’s value, which we’re doing here:

https://github.com/apollographql/graphql-tools/blob/ec6927b197b0b822fb58b5f5e616578ecba8bfe5/src/generate/addResolveFunctionsToSchema.ts#L98

(and here’s the PR where we introduced these changes: #508)

Now that graphql-js is using an internal and private Map (that’s initiated in the GraphQLEnumType constructor), we can’t override enum values in the same way.

Well, technically we can - replacing

https://github.com/apollographql/graphql-tools/blob/ec6927b197b0b822fb58b5f5e616578ecba8bfe5/src/generate/addResolveFunctionsToSchema.ts#L98

with

const enumField = type.getValue(fieldName);
const originalEnumValue = enumField.value;
const newEnumValue = resolverValue[fieldName];
if (hasOwn.call(type, '_valueLookup')) {
  (type as any)._valueLookup.delete(originalEnumValue);
  (type as any)._valueLookup.set(newEnumValue, enumField);
}
enumField.value = newEnumValue;

fixes our problems, but as you can see that means we have to tie into non-public aspects of the GraphQLEnumType class (the _valueLookup instance in particular). Not ideal 🙁.

I'd love to get your thoughts/comments on this @IvanGoncharov . I doubt you'll want to open things up to expose _valueLookup as part of your public API, but would love to know if you have any other ideas around how we can preserve our enum internal value capabilities, with graphql 14.x. We're likely going to run with the patch I listed above for now (and will pin to an exact graphql version as a dependency of graphql-tools), but will continue to look for a better approach. Also, if there's any chance you would be open to adjusting the GraphQLEnumType class to allow enum value changes (to an already created instance), let us know and we'll definitely get a PR in place.

Thanks for your time!

[IvanGoncharov]

Also, if there's any chance you would be open to adjusting the GraphQLEnumType class to allow enum value changes (to an already created instance), let us know and we'll definitely get a PR in place.

@hwillson Problem is that we can't treat GraphQL* objects as mutable since it will break so many assumptions in graphql-js and also makes it impossible to validate such objects.
We simply can't make any changes (including performance optimizations) knowing that any part of a schema can be changed at any moment of time. In the next major release, we will try to make this restriction more explicit by marking more properties and return values as read-only.

But I think core problem here is why you are forced to mutate these object in the first place. My long-term plan is to allow supplying enum values, resolvers, etc. to buildSchema/extendSchema functions similar to makeExecutableSchema so you would be able to attach such values directly to SDL without a need to manipulate the internal representation of GraphQL* objects.

Meanwhile, the best possible solution would be to treat enum value assignment and all other mutation of GraphQL* object as schema transformations.
So you need to convert GraphQLEnumType to GraphQLEnumTypeConfig and add values and create a new GraphQLEnumType instance.

AFAIK, you already have this code here: https://github.com/apollographql/graphql-tools/blob/master/src/stitching/schemaRecreation.ts
I'm not very familiar with the internals of graphql-tools but maybe you can also use this high-level function: https://www.apollographql.com/docs/graphql-tools/schema-transforms.html#transformSchema

Note: We aware of how complex it's to implement schema transformation ATM so we also working on making it more easy to do this, for example:
graphql/graphql-js#1331
graphql/graphql-js#1199

And after stabilizing ecosystem after 14.0.0 I will focus back on the quality of life improvements like these ones.

[IvanGoncharov]

We're likely going to run with the patch I listed above for now (and will pin to an exact graphql version as a dependency of graphql-tools)

@hwillson Please pin it to 14.0.x since 14.1.x and other future feature releases can change some internal APIs and data representations in order to improve the CPU
performance, memory usage for big schemas or to implement some new features.

[IvanGoncharov]

@hwillson We are always publishing RC before any major change so I can notify your or someone else from Apollo team next time when we will publish a new one. So you would have time to test it and give a feedback before public release.

Are you interested and what the best way to do this?
Should I open an issue in this repo or should I send an email to someone from your team?

[hwillson]

Thanks very much for the detailed response @IvanGoncharov! This all makes complete sense. I started heading down the create a new GraphQLEnumType instance approach right before I posted my questions, so I'll keep looking into that a bit further. We're under a bit of a time crunch to get this resolved, so we'll likely have to use the _valueLookup hack for now. Not ideal, but we'll circle back and get this properly resolved after the next release of graphql-tools.

Regarding #945 (comment), that would be awesome! Opening an issue in this repo is probably the best bet, but I'm also subscribed to graphql-js notifications and read them all, so I'll hopefully notice it that way as well.

Thanks again for your help @IvanGoncharov!

[IvanGoncharov]

@hwillson Feel free to ping me if you will face any other issues with porting to a new version of graphql-js.
Also if you need some new APIs or any changes to existing ones, please open an issue on graphql-js repo and I will try to implement it. Having feature-reach public APIs will help to improve the stability across the entire graphql ecosystem.

[IvanGoncharov]

BTW. After every commit we publish the current snapshot of the master as NPM package here:
https://github.com/graphql/graphql-js#want-to-ride-the-bleeding-edge
So you can try it out if you want to check what changed after a certain commit.