compat: minimal changes to support graphql@15 within Apollo Server #1284

abernix · 2020-02-17T19:27:00Z

Some minimal compatibility changes (see commits for additional clarity) to
prepare for graphql@15, which is currently in release candidate stages.

I should note that this PR doesn't attempt to get close to the more
comprehensive set of changes which @yaacovCR's
#1206 brings.

Instead, I'm aiming to provide the minimum compatibility with the least number of breaking changes
in order to continue to support Apollo Server v2, which completely re-exports
the current graphql-tools@4 version.

Future versions of Apollo Server will lessen their reliance on this
graphql-tools package and will also deprecate older versions of graphql
which are still supported by it today (including 0.13.x versions). In the
meantime, I hope this will allow the v2 series of Apollo Server to make it
through graphql@15. (I do not have this same expecation for graphql@16,
and we expect to make changes in Apollo Server 3.x which speak to that.)

I've copied and pasted some of my commit log messages here which are included
in this PR:

66d73a8 (Jesse Rosenberger, 5 hours ago)
   compat: filter extensions prior to passing to `buildASTSchema`.

   .@IvanGoncharov originally brought this to my attention when he pointed me
   to 
   https://github.com/yaacovCR/graphql-tools-fork/issues/32#issuecomment-571252686 
   and suggested stripping extension nodes prior to invoking `buildASTSchema` 
   as a cross-version (v14 <=> v15) approach for interim compatibility on the 
   v4 series of `graphql-tools`.

   The most urgent and pertinent need here from my perspective is to allow 
   user-exploration of the new `graphql@15` release candidate within Apollo 
   Server which currently re-exports the entirety of `graphql-tools` (even 
   though it only relies on small portions of it).

   Upon further investigation of the above-referenced issue, it appears that
   @yaacovCR had already crafted the solution that @IvanGoncharov had
   suggested to me, which I found in 2280eef8ad6c1cbc90c640228eb68094546f60f9
   within the well-organized
   https://github.com/apollographql/graphql-tools/pull/1206
   (which I am thankful for the continued updates on!).

   My commit here merely grabs a sub-set of that commit that seemed most 
   pertinent; I certainly don't claim that this solution is nearly as 
   comprehensive as the original 2280eef8ad6c1cbc90c640228eb68094546f60f9.  My 
   hope is that by using the same code/implementation here, it will marginally 
   lessen future merge conflicts.

   Since this is basically a re-working of @yaacovCR's commit, I've attributed 
   co-authorship of this commit accordingly.  (Thank you, again!)

   Ref: https://github.com/apollographql/graphql-tools/issues/1272 
   Co-authored-by: yaacovCR <yaacovCR@gmail.com>

c7f5088 (Jesse Rosenberger, 5 hours ago)
   Fix incompatibility between `iterall` and newer TypeScript types.

   This wouldn't be necessary if this project had a `package-lock.json`,
   but...

713ffe2 (Jesse Rosenberger, 6 hours ago)
   Inline `PrintSchemaOptions` type, whose parent module has been moved.

   This option will likely be deprecated in `graphql@16`.  For now, we'll 
   inline this type into this module to continue emitting it into the 
   declaration file for `makeRemoteExecutableSchema`.

   This is necessary since the TypeScript compiler can no longer resolve its 
   previous location as `graphql` has moved the location of the
   `schemaPrinter` module to `printSchema` in
   https://github.com/graphql/graphql-js/pull/2426.

@IvanGoncharov

This option will likely be deprecated in `graphql@16`. For now, we'll inline this type into this module to continue emitting it into the declaration file for `makeRemoteExecutableSchema`. This is necessary since the TypeScript compiler can no longer resolve its previous location as `graphql` has moved the location of the `schemaPrinter` module to `printSchema` in graphql/graphql-js#2426. cc @IvanGoncharov

This wouldn't be necessary if this project had a `package-lock.json`, but...

@IvanGoncharov

.@IvanGoncharov originally brought this to my attention when he pointed me to yaacovCR#32 (comment) and suggested stripping extension nodes prior to invoking `buildASTSchema` as a cross-version (v14 <=> v15) approach for interim compatibility on the v4 series of `graphql-tools`. The most urgent and pertinent need here from my perspective is to allow user-exploration of the new `graphql@15` release candidate within Apollo Server which currently re-exports the entirety of `graphql-tools` (even though it only relies on small portions of it). Upon further investigation of the above-referenced issue, it appears that @yaacovCR had already crafted the solution that @IvanGoncharov had suggested to me, which I found in 2280eef within the well-organized #1206 (which I am thankful for the continued updates on!). My commit here merely grabs a sub-set of that commit that seemed most pertinent; I certainly don't claim that this solution is nearly as comprehensive as the original 2280eef. My hope is that by using the same code/implementation here, it will marginally lessen future merge conflicts. Since this is basically a re-working of @yaacovCR's commit, I've attributed co-authorship of this commit accordingly. (Thank you, again!) Ref: #1272 Co-authored-by: yaacovCR <yaacovCR@gmail.com>

trevor-scheer

Minor comments, non-blocking. LGTM!

trevor-scheer · 2020-02-17T19:47:52Z

src/generate/filterExtensionDefinitions.ts

+import { DocumentNode, DefinitionNode, Kind } from 'graphql';
+
+export default function filterExtensionDefinitions(ast: DocumentNode) {
+  const extensionDefs = ast.definitions.filter(


graphql-js actually provides us this exact filter predicate via isTypeSystemExtensionNode, can we use that here instead?

I considered that, but that predicate wasn't introduced until, I think, 14.0.0. I do think that any version of graphql-tools that has narrowed its peerDependencies to exclude pre-14.x should be using that predicate though!

trevor-scheer · 2020-02-17T19:56:34Z

src/stitching/makeRemoteExecutableSchema.ts

+ * This type has been copied inline from its source on `@types/graphql`:
+ *
+ * https://git.io/Jv8NX


@types/graphql is no longer being kept up, but it seems the same type is available in the source still? We could also copy/reference from there if not, so the reference is current.

https://git.io/Jv4tt

Aside from the name, the reference is current. Referencing it from the new location could be a problem for cross-graphql version compatibility, so keeping it in line here seems okay for now.

yaacovCR · 2020-02-18T04:52:24Z

Is this considered minimally supporting graphql v15 without supporting interfaces that implement interfaces?

These changes would support graphql v14 compliant schemas running on higher versions of graphql. Still important, of course, but not the same thing.

It was not incredibly hard to add support for interfaces implementing interfaces, I think you should consider it.

abernix · 2020-02-19T11:29:46Z

@yaacovCR That's correct. I think that supporting newer graphql@15 features in graphql-tools will be something that your PR is best at handling, but I'm certainly willing to bring in the support for interfaces implementing interfaces if it's relatively straightforward.

The intent/motivation here from my perspective is to make sure that when v15 comes out and folks who are using Apollo Server (which is responsible for a substantial portion of the downloads of graphql-tools) upgrade to it, things mostly keep working just the same as they were.

Keeping things just the same seems like a good first step since, while end-users might have compelling reason to update to graphql@15 themselves (e.g. new features), the more pressing concern is that they upgrade decision might be made for them. For example, a GraphQL CLI tool which has a dependency on graphql may update its own non-peer-dependency and, because of the age-old un-resolved issue of having multiple copies of graphql in a project, may force that upgrade on its users, even if they aren't declaring interfaces implementing interfaces. Plus, various dependency update bots (Greenkeeper, Renvoate, etc.) may automatically make that upgrade happen. In these cases, I think it's ideal if graphql@14-compatible schemas keep working with graphql@15.

Anyhow, all of that said, I still want to consider bringing in support for interfaces implementing interfaces to graphql-tools if it's relatively straightforward, and you seem like the expert in having made this assessment. Would that merely require bringing in d36029f?

I'll merge and release this for now, but happy to open another small PR that brings that long so long as it's compatible with the peerDependencies versions that graphql-tools and Apollo Server still supports. (And to be clear, I'm personally very excited to de-couple the cross-package reliances here and make things more flexible in some new major versions which deprecate old behavior that are still being clung onto, but I don't think we're quite there yet in terms of how this package is relied upon.)

yaacovCR · 2020-02-19T14:05:09Z

You can't just copy changes in terms of interfaces implementing interfaces from the fork because of my unification of the different transform schema processes. You just have to go through the different places where interfaces are accessed or set up on objects and make sure that they are also done so on interfaces as well. You can use my changes as guide, however.

yaacovCR · 2020-02-27T05:36:11Z

@abernix

yaacovCR@9ed6fc4

graphql-tools-fork will now support v0.12, v0.13, v14, and v15.

Will add as well to #1206

What other blockers are there to merging #1206?

abernix and others added 3 commits February 17, 2020 21:13

Fix incompatibility between iterall and newer TypeScript types.

c7f5088

This wouldn't be necessary if this project had a `package-lock.json`, but...

trevor-scheer approved these changes Feb 17, 2020

View reviewed changes

Add CHANGELOG for #1284.

643d2c9

abernix merged commit a9965ec into master Feb 19, 2020

abernix deleted the abernix/graphql-15 branch February 19, 2020 11:36

This was referenced Feb 27, 2020

use graphql-tools-fork ardatan/graphql-toolkit#460

Merged

graphql v15 will break graphql-tools #1272

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

compat: minimal changes to support graphql@15 within Apollo Server #1284

compat: minimal changes to support graphql@15 within Apollo Server #1284

abernix commented Feb 17, 2020

trevor-scheer left a comment

trevor-scheer Feb 17, 2020

abernix Feb 19, 2020

trevor-scheer Feb 17, 2020

abernix Feb 19, 2020

yaacovCR commented Feb 18, 2020

abernix commented Feb 19, 2020

yaacovCR commented Feb 19, 2020 •

edited

yaacovCR commented Feb 27, 2020 •

edited

compat: minimal changes to support graphql@15 within Apollo Server #1284

compat: minimal changes to support graphql@15 within Apollo Server #1284

Conversation

abernix commented Feb 17, 2020

trevor-scheer left a comment

Choose a reason for hiding this comment

trevor-scheer Feb 17, 2020

Choose a reason for hiding this comment

abernix Feb 19, 2020

Choose a reason for hiding this comment

trevor-scheer Feb 17, 2020

Choose a reason for hiding this comment

abernix Feb 19, 2020

Choose a reason for hiding this comment

yaacovCR commented Feb 18, 2020

abernix commented Feb 19, 2020

yaacovCR commented Feb 19, 2020 • edited

yaacovCR commented Feb 27, 2020 • edited

yaacovCR commented Feb 19, 2020 •

edited

yaacovCR commented Feb 27, 2020 •

edited