mswjs / msw

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

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

Closed kettanaito closed 2 months ago

kettanaito commented 5 months ago

Changes

mattcosta7 commented 3 months ago

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 commented 3 months ago

@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 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.

Lalem001 commented 2 months ago

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 commented 2 months ago

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 commented 2 months ago

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 commented 2 months ago

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 commented 2 months ago

@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 commented 2 months ago

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.