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

feat: list "graphql" as an optional peer dependency #2187

Merged
merged 4 commits into from
Aug 28, 2024

Conversation

kettanaito
Copy link
Member

Changes

  • Removes graphql as a regular dependency.
  • Adds graphql as a peer dependency of MSW.
  • Adds graphql to the peerDependenciesMeta as an optional peer dependency (similar to typescript).

@mattcosta7
Copy link
Contributor

nice! this is safe since we removed the line that parsed graphql to make suggested handlers, right? (I think we did, but can't recall exactly)

@kettanaito
Copy link
Member Author

kettanaito commented Aug 6, 2024

@mattcosta7, thanks for getting your eyes on this. Yes, this should be safe!

There's an effort to improve the unhandled requests logging but I advocated not to rely on GraphQl there, only to parse the body (#2227). I would love to include this pull request in the next minor release. For now, will see if there are any issues we should fix first...

@kettanaito kettanaito merged commit 40b17fd into main Aug 28, 2024
12 checks passed
@kettanaito kettanaito deleted the feat/graphql-peer-dep branch August 28, 2024 16:26
@kettanaito
Copy link
Member Author

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.

@Lalem001
Copy link
Contributor

Lalem001 commented Aug 28, 2024

I am not sure this was a safe change. In my project, vitest is erroring out because it can no longer import graphql. I don't use graphql in my setup at all, so I was surprised to see the error.

2:54:25 PM [vite] Internal server error: Failed to resolve import "graphql" from "node_modules/msw/lib/core/utils/internal/parseGraphQLRequest.mjs?v=5460f706". Does the file exist?
  Plugin: vite:import-analysis
  File: /project/node_modules/msw/lib/core/utils/internal/parseGraphQLRequest.mjs?v=5460f706:6:22
  1  |  import { parse } from "graphql";

I do not know why parseGraphQLRequest.mjs is being imported.

Installing graphql on my own resolves the issue, but essentially makes it a required dependency in my setup. Not optional. Having looked at the original issue, I see why it should be a peer dep of MSW, but it still broke at least one setup.

@Greg-Smulko
Copy link

It fails for us as well:

node_modules/msw/lib/core/GraphQLHandler-Cu4Xvg4S.d.ts(1,63): error TS2307: Cannot find module 'graphql' or its corresponding type declarations.
node_modules/msw/lib/core/graphql.d.ts(1,30): error TS2307: Cannot find module 'graphql' or its corresponding type declarations.

We also don't use graphql at all.

@topaxi
Copy link

topaxi commented Aug 29, 2024

Same here, imported in the node process, maybe via bundler and tree-shaking this might not be an issue. But imports via node crash due to missing modules.

@kettanaito
Copy link
Member Author

Thanks for reporting this! For now, you can revert to the previous minor version to not deal with this error.

I believe graphql gets imported if you are using a single import:

import { http } from 'msw'

This imports graphql because not all tooling you use does tree-shaking or even bundling.

One way to fix this is to use granular imports, which we've shipped some time back:

import { http } from 'msw/core/http'

Let me know if this helps!

@Akallabet
Copy link

Akallabet commented Aug 29, 2024

@kettanaito thanks for looking into it.
In my case this is a pretty big deal as this change is blocking our CI, we use msw in most of our tests.
I appreciate there's a potential workaround using granular imports, but it will require changes in quite few repositories.
Would it be possible to just revert this change?

@kettanaito
Copy link
Member Author

My comment above mentions you should downgrade if this affects you. Please do. That's handling open source issues 101 ;)

The fix is already open at #2250. I will release it once the build is complete.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

List "graphql" as a peer dependency
6 participants