Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

List "graphql" as a peer dependency #2185

Open
4 tasks done
alessbell opened this issue Jun 18, 2024 · 5 comments · Fixed by #2187
Open
4 tasks done

List "graphql" as a peer dependency #2185

alessbell opened this issue Jun 18, 2024 · 5 comments · Fixed by #2187
Labels
api:graphql dependencies Pull requests that update a dependency file feature needs:discussion

Comments

@alessbell
Copy link

Prerequisites

Environment check

  • I'm using the latest msw version
  • I'm using Node.js version 18 or higher

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: graphql/graphql-js#594 (comment)

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 alessbell added bug Something isn't working needs:triage Issues that have not been investigated yet. scope:browser Related to MSW running in a browser labels Jun 18, 2024
@alessbell
Copy link
Author

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 (ardatan/graphql-tools#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
Copy link
Member

kettanaito commented Jun 21, 2024

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 kettanaito added feature api:graphql needs:discussion dependencies Pull requests that update a dependency file and removed bug Something isn't working scope:browser Related to MSW running in a browser needs:triage Issues that have not been investigated yet. labels Jun 21, 2024
@kettanaito kettanaito changed the title Remove graphql as a direct dependency in favor of a peer dependency List "graphql" as a peer dependency Jun 21, 2024
@kettanaito
Copy link
Member

I've opened a pull request with the proposed change: #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
Copy link
Member

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
Copy link
Member

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api:graphql dependencies Pull requests that update a dependency file feature needs:discussion
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants