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.

Sign up for GitHub

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

Jump to bottom

List "graphql" as a peer dependency #2185

Open
4 tasks done
alessbell opened this issue Jun 18, 2024 · 3 comments · May be fixed by #2187
Open
4 tasks done

List "graphql" as a peer dependency #2185

alessbell opened this issue Jun 18, 2024 · 3 comments · May be 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#863 (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 •
edited
Loading

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 kettanaito linked a pull request Jun 21, 2024 that will close this issue
Open
@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!

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

Successfully merging a pull request may close this issue.

feat: list "graphql" as an optional peer dependency mswjs/msw
2 participants
@alessbell @kettanaito