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

fix: add graphql to "peerDependencies" #2249

Merged
merged 1 commit into from
Aug 29, 2024
Merged

Conversation

THETCR
Copy link
Contributor

@THETCR THETCR commented Aug 29, 2024

@kettanaito kettanaito changed the title fix(build): add graphql to peerDependencies fix: add graphql to "peerDependencies" Aug 29, 2024
Copy link
Member

@kettanaito kettanaito left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for fixing this! 👏

@kettanaito kettanaito merged commit 8a9568a into mswjs:main Aug 29, 2024
1 check passed
@kettanaito
Copy link
Member

Released: v2.4.1 🎉

This has been released in v2.4.1!

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.

@pascalmann
Copy link

pascalmann commented Aug 29, 2024

Not work for me.
grapql was added to devDependencies (so no installed in my repo), but is still imported (lazily), even if I not use it.

@thepassle
Copy link
Contributor

thepassle commented Sep 4, 2024

This should've been a breaking change (and it did cause breakage for some of our projects). If people import something from 'msw', it'll still hit the dynamic graphql import (even if we don't use anything graphql related from msw), which now doesnt get automatically installed because its only a peer dependency.

Thankfully I can work around it by rewriting our import from 'msw' to 'msw/core/http', since we only use http and not graphql, but more people will start running into this issue as well.

@kettanaito
Copy link
Member

If people import something from 'msw', it'll still hit the dynamic graphql import (even if we don't use anything graphql related from msw)

This is the part that's confusing. How? Dynamic imports are by definition a part of your runtime. I don't suppose the browser extracts that dynamic import, puts it on the root scope, and then evaluates it. That mustn't happen, otherwise I see no point in a dynamic import as a feature.

Removing a graphql dependency is fix by design. You shouldn't be installing that package if you aren't planning on mocking anything GraphQL. As such, it was shipped as fix. The fact that the first few iterations of that fix are problematic in certain environments is a separate discussion.

I am welcoming reproduction repos at this point because none of my prior testing caught this. Here's where I've tested these fixes:

  • Vitest ESM
  • Vitest CJS
  • Jest ESM
  • Jest CJS
  • webpack-based project

I hope that anybody reaching out understands these changes are done in good faith. I cannot physically test every existing combination of your tooling. Yes, things may break. You are more than welcome to pin the MSW version and go on with your life. If not, you should contribute with discussions, suggestions, and pull requests. Thanks.

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

Successfully merging this pull request may close these issues.

4 participants