search

mercurius-js / mercurius

Implement GraphQL servers and gateways with Fastify
https://mercurius.dev/
MIT License
2.33k stars 234 forks source link

Fix: remove graphql as a dependency #956

Closed MaartenRimaux closed 1 year ago

MaartenRimaux commented 1 year ago

Current behaviour

As stated in this PR: https://github.com/mercurius-js/mercurius/pull/711

Expected behaviour

graphql should only be listed as a peerDependency otherwise it is possible that you have two instances of graphql (Explanation can be found here: https://github.com/mercurius-js/mercurius/pull/711#issuecomment-1030572248).

MaartenRimaux commented 1 year ago

@mcollina who should I add as a reviewer?

mcollina commented 1 year ago

The fundamental problem is the inconsistent behavior of peerDependency across different package managers. We can safely do this once we drop support for Node v14 / npm 6 in a few months once v14 goes out of support.

This was changed in npm v7, and it mostly matches what the other package managers are doing: https://github.com/npm/rfcs/blob/main/implemented/0025-install-peer-deps.md.

MaartenRimaux commented 1 year ago

@mcollina, thank you for the explanation! What do you suggest for people who are having issues with two instances of the graphql package in the meantime?

mcollina commented 1 year ago

What do you suggest for people who are having issues with two instances of the graphql package in the meantime?

The only time this happened to me was when there were two incompatible versions installed at the same time, like GraphQL v15 and v16. The solution was evident in that case.

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.

Happy to review a repository that reproduces the problem.

MaartenRimaux commented 1 year ago

@mcollina, I created a reproduction repository but couldn't reproduce the error. This eventually led me to find the real error in my own code. Anyway, thank you for your help!