mswjs / msw

Industry standard API mocking for JavaScript.
https://mswjs.io
MIT License
15.99k stars 521 forks source link

List "graphql" as a peer dependency #2185

Open alessbell opened 5 months ago

alessbell commented 5 months ago

Prerequisites

Environment check

Browsers

No response

Reproduction repository

https://github.com/apollographql/graphql-testing-library

Reproduction steps

When installing msw, graphql is installed as a transitive dependency at v16. This goes against the expectation in the GraphQL ecosystem that graphql always be treated as a peer dependency: https://github.com/graphql/graphql-js/issues/594#issuecomment-1189850052

From the graphql source:

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.

This cannot be trivially overridden with all package managers (e.g. npm overrides has open issues), and causes surprising behavior.

Current behavior

graphql is a dependency of MSW.

Here's an example of a project using a number of packages that rely on graphql. All of them list it as a peer dependency with the range of 15 || 16, except for MSW.

CleanShot 2024-06-18 at 15 10 56

The result is my MSW GraphQL handler returning errors about unsupported GraphQL features for a version of GraphQL I haven't installed and don't expect to be executing my queries: CleanShot 2024-06-18 at 15 12 34

Expected behavior

I'd expect MSW to treat it as a peer dependency.

alessbell commented 5 months ago

This error was a red herring 🙃 It's actually coming from @graphql-tools/executor which lists graphql as a peer dependency that includes v15 in its range, but copy-pasted execute.ts from graphql v16 into its src (https://github.com/ardatan/graphql-tools/issues/6279).

The fact that MSW lists graphql as a dependency was unexpected, though; it would be easier to rule it out as a source of potential conflicts if it treated it as a peer dependency, so I'll leave this open for now.

kettanaito commented 5 months ago

Hi, @alessbell. Thanks for proposing this.

I agree it makes sense to list graphql as a peer dependency of MSW. I wouldn't allow both v15 and v16 because our library code still has to (dev) depend on a fixed version of GraphQL. As of now, we support v16 officially, with no promise of v15 support. I think that's an important note given the rest of your repo relies on v15.

One potential problem here is that graphql is an optional peer dependency. While there's peerDependenciesMeta field in package.json, it's up to your package manager to interpret or even respect that field. Optional peer dependencies are still rather poorly supported in the ecosystem last time I checked. I am a bit concerned that some folks will start getting a missing peer dependency warning if we make this change. Granted, that likely means their package manager is old and doesn't support the meta field.

kettanaito commented 5 months ago

I've opened a pull request with the proposed change: https://github.com/mswjs/msw/pull/2187.

@alessbell, please, could you give it a try in your project? You can add a dependency from a pull request/branch using all modern package managers. If you have any problems, please let me know.

Code reviews are welcome too!

kettanaito commented 2 months ago

Released: v2.4.0 🎉

This has been released in v2.4.0!

Make sure to always update to the latest version (npm i msw@latest) to get the newest features and bug fixes.


Predictable release automation by @ossjs/release.

kettanaito commented 2 months ago

My attempt at listing graphql as an optional peer dependency was unsuccessful. There isn't really a concept of an optional peer dependency in ESM (see #2267 for more details).

As the next step, I will try installing graphql as a regular dependency with MSW while also listing it as an optional peer dependency. That should provide both the fallback for teams not using GraphQL, as well as take the project's graphql version as a priority during the module resolution. At least, I hope it will.