fix(): move graphql dependency back to dev deps

[kamilmysliwiec]

Current behavior

I've just noticed that the graphql package is currently defined as both "dependency" and a "peerDependency" (at the same time) which implicitly means it's no longer a "peer dependency" as it will be auto-installed by package managers as an internal mercurius' dependency.

This has been already fixed in the past but it was reverted back in this PR #565

Expected behavior

graphql should be listed as a devDependency so it gets auto-installed for those who want to contribute. For package consumers though, it should be defined as a peer dependency, otherwise, you may run into the following issue:

Error: Cannot use GraphQLSchema "{ __validationErrors: [], description: undefined, extensions: undefined, astNode: undefined, extensionASTNodes: [], _queryType: Query, _mutationType: Mutation, _subscriptionType: Subscription, _directives: [@include, @skip, @deprecated, @specifiedBy], _typeMap: { Recipe: Recipe, ID: ID, String: String, Date: Date, Query: Query, Int: Int, Mutation: Mutation, NewRecipeInput: NewRecipeInput, Boolean: Boolean, Subscription: Subscription, __Schema: __Schema, __Type: __Type, __TypeKind: __TypeKind, __Field: __Field, __InputValue: __InputValue, __EnumValue: __EnumValue, __Directive: __Directive, __DirectiveLocation: __DirectiveLocation }, _subTypeMap: {}, _implementationsMap: {} }" from another module or realm.

Ensure that there is only one instance of "graphql" in the node_modules
directory. If different versions of "graphql" are the dependencies of other
relied on modules, use "resolutions" to ensure only one version is installed.

https://yarnpkg.com/en/docs/selective-version-resolutions

Duplicate "graphql" modules cannot be used at the same time since different
versions may have different capabilities and behavior. The data from one
version used in the function from another could produce confusing and
spurious results.

Great work on this package @mcollina!

[mcollina]

I don't remember what was the actual problem but it did not work as expected across the various package managers and node versions.

@simone-sanfratello did you recall what was the problem you had?

[simone-sanfratello]

pretty sure it loads the graphql v15 from graphql-jit, instead of the actual v16 required

install graphql v16 as dependency should fix it

[kamilmysliwiec]

@simone-sanfratello why not leave the graphql as a peer dependency and instruct people to install the appropriate version of GraphQL then? Specifying graphql as a dependency will lead to issues with all packages that also depends on the graphql package (so basically any plugins, GQL playgrounds, libs, etc.)

Also, assuming that this package requires GraphQL^16, package managers should be smart enough to not pull GraphQL v15 from graphql-jit if the peer dependency is specified as follows: "graphql": "^16.0.0"

[simone-sanfratello]

which version of npm are you using? Not sure, but it works in different ways in v6 and v7 and v8

[kamilmysliwiec]

Shouldn't this package work for all these NPM versions?

Correct me if I'm wrong but I think we leave out what is most important here - once you define a package as a "dependency" in the package.json file, adding it as a "peerDependency" does not change anything so essentially "graphql" is now registered as an internal dependency of "mercurius" ("peerDependencies" part is ignored in this case). If you look at graphql-tools monorepo (example: https://github.com/ardatan/graphql-tools/blob/master/packages/utils/package.json) every package defines "graphql" as a peer dependency to make sure they all (when used) are using the same version of the core (graphql) package. Not sure how NPM version is relevant to this issue (I suspect graphql package is being automatically hoisted in one of the versions you've mentioned and that's why this issue isn't reproducible everywhere).

Perhaps this is something that should be investigated at the package manager level (to make sure graphql can be safely specified as a peer dependency and won't be auto-hoisted from graphql-jit)

[simone-sanfratello]

@simone-sanfratello why not leave the graphql as a peer dependency and instruct people to install the appropriate version of GraphQL then? Specifying graphql as a dependency will lead to issues with all packages that also depends on the graphql package (so basically any plugins, GQL playgrounds, libs, etc.)

because mercurius v9 specifically requires graphql v16, that's why. It doesn't work with any version of graphql, so it need a strict dependency

so, moving to a dev dep will cause in the final application an unwanted behavior very hard to understand

[kamilmysliwiec]

@simone-sanfratello

because mercurius v9 specifically requires graphql v16, that's why. It doesn't work with any version of graphql, so it need a strict dependency

Having "graphql": "^16.0.0" constraint in the peerDependencies already informs that this package isn't designed to work with older versions of this package. Package managers, depending on the version, will either warn you or print an error message that you should install GraphQL^16 in your project if it's missing (or if you have an older version installed). You shouldn't use "dependencies" for that.

If you look at the graphql-tools repository (I'm using it just as an example), you'll notice that all these packages do require using GraphQL^16 as well (and their latest versions won't work properly with, for example, GraphQL^15). This is also why they have "graphql": "^16.0.0" in peerDependencies, not dependencies(!).

so, moving to a dev dep will cause in the final application an unwanted behavior very hard to understand

How come?

If you're assuming that library users will ignore errors printed out by package managers (which is quite an odd assumption but let's say they do), you can always add an extra runtime check. This is exactly why graphql package exports a version constant so if it's invalid (import { version } from 'graphql'), you can just log the error down.

[simone-sanfratello]

@simone-sanfratello

because mercurius v9 specifically requires graphql v16, that's why. It doesn't work with any version of graphql, so it need a strict dependency

Having "graphql": "^16.0.0" constraint in the peerDependencies already informs that this package isn't designed to work with older versions of this package. Package managers, depending on the version, will either warn you or print an error message that you should install GraphQL^16 in your project if it's missing (or if you have an older version installed). You shouldn't use "dependencies" for that.

If you look at the graphql-tools repository (I'm using it just as an example), you'll notice that all these packages do require using GraphQL^16 as well (and their latest versions won't work properly with, for example, GraphQL^15). This is also why they have "graphql": "^16.0.0" in peerDependencies, not dependencies(!).

so, moving to a dev dep will cause in the final application an unwanted behavior very hard to understand

How come?

If you're assuming that library users will ignore errors printed out by package managers (which is quite an odd assumption but let's say they do), you can always add an extra runtime check. This is exactly why graphql package exports a version constant so if it's invalid (import { version } from 'graphql'), you can just log the error down.

I'll spend some time in the weekend to show how peerDep is not enough, and to check if it works in a different way in npm v6 and above - I guess so, but I'm not sure

[kamilmysliwiec]

Thanks @simone-sanfratello

BTW I just noticed that the graphql-jit also defines graphql as a peer dependency (which is valid)

[simone-sanfratello]

this is the situation as is, with graphql as explicit dependency of mercurius, for an application installed as

npm i fastify mercurius

that's why we need this, without the explicit grapqhql dedendency require('graphql') in mercurius will load the /node_modules/graphql at first level, instead of the current one, because the peerDependency does not solve this.

this is really hard to debug #693

we can require to install graphql explicitly in the final app, as written in the documentation

npm i fastify mercurius graphql

but mercurius works with the precise version of graphql, so it has to be a dependency

note: it works in the same way with npm v6, v7 and v8

[kamilmysliwiec]

because the peerDependency does not solve this.

I think you might have misunderstood what "peerDependency" means. Every peer dependency must be explicitly installed to ensure that the same behavior is encountered between different package managers.

but mercurius works with the precise version of graphql, so it has to be a dependency

graphql shouldn't be declared as a dependency, even if Mercurius works only with GraphQL >= 16 and not with v15. Mercurius also does not use a "precise version of graphql" https://github.com/mercurius-js/mercurius/blob/master/package.json#L63 (^ = every patch and minor release will be allowed either way).

this is really hard to debug #693

In this issue, you mentioned "the issue is caused by using graphql-jit that uses graphql@15" which isn't correct: https://github.com/zalando-incubator/graphql-jit/blob/main/package.json#L49 (please note ">="). graphql-jit even showcases benchmarks with GraphQL v16 in their README (https://github.com/zalando-incubator/graphql-jit#benchmarks)

If you still disagree, please, feel free to close this PR. I feel like I've already spent enough time explaining why the current configuration is invalid.

[simone-sanfratello]

I'm not a maintainer of this module, but I guess your PR is not going to be merged because it doesn't pass tests.

You were right if graphql were installed under each node_modules dir of both mercurius and graphql-jit, since they have 2 incompatible versions, but because this is not happening, and for some reason, the version of graphql-jit is installed in the root of node_modules, this cause problem especially in the final app.

Even worse, having graphql as a peer dependency of mercurius cause this (tested with npm v6 and v8)

Now we can endlessly be arguing about how peer dependencies should work and how actually work, but the fact that code must work remain.

[mcollina]

Something that would be useful is to have a repro where the current setup would fail.

[SimenB]

Hiya 👋 I've spent way much time fighting peer dependency errors, so happy to lend a hand here. I think the flip side of what Matteo says ("a reproduction for when current approach fails") should be the next step. When does having it as a peer fail. The issue with the current approach (as has been discussed in this issue) is that the peer dependency essentially does nothing, and what you want is for users to provide their own version of graphql.

One concrete example is #711 (comment), which is fixed by zalando-incubator/graphql-jit#156. If there are other cases (unfortunately, tracking down peer dependency errors is often an exercise in whack-a-mole) we should fix those as they crop up.

---

As an example, running yarn in this repo results in

warning "graphql-jit > @graphql-typed-document-node/core@3.1.0" has incorrect peer dependency "graphql@^0.8.0 || ^0.9.0 || ^0.10.0 || ^0.11.0 || ^0.12.0 || ^0.13.0 || ^14.0.0 || ^15.0.0".
warning " > @typescript-eslint/eslint-plugin@4.33.0" has unmet peer dependency "eslint@^5.0.0 || ^6.0.0 || ^7.0.0".
warning "@typescript-eslint/eslint-plugin > @typescript-eslint/experimental-utils@4.33.0" has unmet peer dependency "eslint@*".
warning "@typescript-eslint/eslint-plugin > @typescript-eslint/experimental-utils > eslint-utils@3.0.0" has unmet peer dependency "eslint@>=5".
warning " > @typescript-eslint/parser@4.33.0" has unmet peer dependency "eslint@^5.0.0 || ^6.0.0 || ^7.0.0".
warning "graphql-tools > @apollo/client@3.4.17" has incorrect peer dependency "graphql@^14.0.0 || ^15.0.0".

So we have warnings for eslint and graphql. I'll keep eslint out (as it's a dev dep and doesn't affect consumers (that said, just updating to @typescript-eslint at v5 will fix it no, standard is used here)), but the two others shows the issue. graphql-jit is fixed by the PR I've linked, and graphql-tools should be deleted, as, from what I can tell, it's unused.

$ rg graphql-tools
examples/executable-schema.js
5:const { makeExecutableSchema } = require('@graphql-tools/schema')

docs/api/options.md
335:const { makeExecutableSchema } = require('@graphql-tools/schema')

package.json
32:    "@graphql-tools/merge": "^8.0.0",
33:    "@graphql-tools/schema": "^8.0.0",
34:    "@graphql-tools/utils": "^8.0.0",
44:    "graphql-tools": "^8.0.0",

test/app-decorator.js
12:const { makeExecutableSchema } = require('@graphql-tools/schema')

test/directives.js
7:const { makeExecutableSchema } = require('@graphql-tools/schema')
8:const { mergeResolvers } = require('@graphql-tools/merge')
15:} = require('@graphql-tools/utils')

test/hooks.js
6:const { mapSchema } = require('@graphql-tools/utils')

test/types/index.ts
12:import { makeExecutableSchema } from '@graphql-tools/schema'
13:import { mapSchema } from '@graphql-tools/utils'

[mcollina]

I would go ahead and drop graphql from the peerDependencies.

[SimenB]

That's not the correct solution as there needs to be only one instance of graphql in the dependency tree (see e.g. graphql/graphql-js#3217 (comment) (while it talks about CJS vs ESM, it also highlights why duplicated graphql is bad)). And since there should only be one instance, it needs to come from the consumer, not this library. If it's in dependencies here, you rely on package managers hoisting correctly and consumers deduping.

[mcollina]

And since there should only be one instance, it needs to come from the consumer, not this library.

I do not understand this. What is the goal? "bring your own version" sounds like a maintenance nightmare to me. I do not wish to guarantee support for anything but what Mercurius is tested on.
If you use this module you "buy in" using this specific version of graphql.

In my opinion the whole "bring your version" of graphql does not really work as expected in the ecosystem: the issue on graphql-jit is an example of that. Changing this module would not have prevented that problem either.

Specifying it as a dependency has the benefit of making the install of graphql automatic for npm v6 users, which is still the default version of npm in Node v14 and its usage is still very widespread. This specific problem will disappear with node v16.

[kamilmysliwiec]

I do not understand this. What is the goal? "bring your own version" sounds like a maintenance nightmare to me. I do not wish to guarantee support for anything but what Mercurius is tested on.

As I've mentioned in my comments above, there're several ways of specifying what versions are supported (and tested) without setting graphql as a dependency (setting version range in the peerDependencies entry, adding an extra check at runtime, etc.).

[mcollina]

I'm very familiar about the mechanisms. I do not understand what use cases this specific way does not cover. Is it a yarn1 issue? a yarn2 issue? a pnpm issue? is it a typescript issue?

If all dependencies agrees on the same version range, then the package manager hoist the dependency and it's one and the same everywhere.

The current code works and all the tests are passing with the default version of the package manager in all supported runtimes.
I have been hit several times by the version mismatch runtime check in graphql but never with the current setup.

How can I reproduce a situation where it fails?

[rpvsilva]

Any news about this issue? I'm getting the same error 😕

[mcollina]

Update your dependencies and the error should disappear.

[voxpelli]

That's not the correct solution as there needs to be only one instance of graphql in the dependency tree (see e.g. graphql/graphql-js#3217 (comment) (while it talks about CJS vs ESM, it also highlights why duplicated graphql is bad)). And since there should only be one instance, it needs to come from the consumer, not this library. If it's in dependencies here, you rely on package managers hoisting correctly and consumers deduping.

I second this and here's why:

• Using npm@9 I have graphql as a dependency in my project so that I can reference its types
• Then when I installed the latest mercurius it did not complain about my graphql being version 15.x but simply went ahead and installed its conflicting graphql@16 within mercurius/node_modules/graphql, as per npm dedupe logic
• This caused tricky type errors as my own code was now referencing a different major version GraphQL types to what mercurius were referencing
• It also caused graphql@15 to wrongfully be what was actually being run, as graphql-jit was not installed to mercurius/node_modules/graphql-jit (as it had no conflicting dependencies with the ones I had installed that forced it down there, and the peer dependency of it is compliant with graphql@15) and thus would find my graphql rather than that of mercurius

I would go ahead and drop graphql from the peerDependencies.

This would not solve above issue as graphql-jit will use whatever is at the top level node_modules, not whatever is at mercurius/node_modules

It would also not solve the issue of incompatible types between my directly referenced grapqhl library and that of mercurius – unless mercurius opts to re-export all of the types of graphql from its own local graphql (which would force everyone who references graphql types to instead reference mercurius to avoid conflicts)

From @mcollina in #956 (comment):

pnpm, npm and yarn are all hoisting graphql correctly if the version range match. You might have to regenerate the lock files / clean up node_modules.

This is true, and peerDependencies is the mechanism used to instruct pnpm, npm and yarn etc that graphql should be hoisted to the top.

With graphql being a dependency rather than a peerDependency that is not enforced, at least not in npm@9.

So:

• Because of graphql-jit (at least how its used now) there is a need for mercurius to use peerDependencies instead of dependencies for graphql right now, else correct functionality can not be guaranteed
• To make it feasible to work with GraphQL types together with mercurius the latter should push a single installed version and as eg. @SimenB points out, the only way to enforce that is through peerDependencies

[mcollina]

Ok, let's do the peerDependendencies.

[mcollina]

This shipped