fix(deps): include "graphql" as a dependency #1381
Conversation
this makes the graphql peer dependency required. While the hope is to eventually avoid the need to require the graphql library for users who don't declare graphql requests directly, the current architecture means that many tools fails to properly compile without error/warning without this. Since NPM 7, npm will automatically install peer dependencies, which should avoid consumers of msw having to manually install graphql. re mswjs#1371
|
This pull request is automatically built and testable in CodeSandbox. To see build info of the built libraries, click here or the icon next to each commit SHA. Latest deployment of this branch, based on commit b26c2f1:
|
removes the previous attempt at making graphql optional by lazily requiring it instead of importing it. This didn't have the intended affect of helping to avoid parsing in many environments, and might be confusing and/or cause friction with a future migration to using native modules and providing an export that can be treeshaken in all environments. re mswjs#1371
mattcosta7
changed the title
There was a problem hiding this comment.
Thanks for tackling this, @mattcosta7!
I've left one proposal about how to move forward with this. I'm curious what you think about this.
| @@ -1,6 +1,5 @@ | |||
| import fetch from 'cross-fetch' | |||
| import { graphql as executeGraphql } from 'graphql' | |||
| import { buildSchema } from 'graphql/utilities' | |||
There was a problem hiding this comment.
we use this from /utilities and from graphql in different files, so I moved to just using the base package export
There was a problem hiding this comment.
💯 agree with this. We should rely on the base package all the time.
|
Failing on typescript 4.5 looks like it was a network blip and I expect it to succeed if we re-run |
|
@mattcosta7, yeap, looks like a hiccup when installing dependencies on CI. Re-running... |
kettanaito
changed the title
|
Thank you so much for your work on this, @mattcosta7! 👏 You're our dependency wizard. |
|
Thanks @mattcosta7! |
|
We need to write this somewhere. @ashtonlance, MSW is released automatically at regular intervals. I believe it's every 3 days: msw/.github/workflows/release.yml Line 5 in 8436515 But since this is a blocking issue for our users I will trigger the release manually right now. |
Released: v0.46.1 🎉This has been released in v0.46.1! Make sure to always update to the latest version ( Predictable release automation by @ossjs/release. |
this makes the graphql peer dependency required. While the hope is to eventually avoid the need to require the graphql library for users who don't declare graphql requests directly, the current architecture means that many tools fails to properly compile without error/warning without this.
Since NPM 7, npm will automatically install peer dependencies, which should avoid consumers of msw having to manually install graphql.
While ideally we'd avoid this now - this PR may be a reasonable stopgap that avoids frustration/confusion in the interim
I also reverted the change to
require('graphql'). The semantics of lazily requiring are a bit off imo, and given that it doesn't provide the desired effect, there's a bit more clarity maintaining import syntax.A follow-up that separates the package/bundle in a way that allows to properly treeshake the graphql imports (or provide them from a different import specifier), so they aren't required dependencies would be ideal.
Totally get it, if the goal is to try a fail forward approach instead of trying to smooth out the current usage, until a more holistic fix can be merged - but figured this might avoid some annoyance in the meantime (both on consumers and avoid issues like #1371 and the handful of duplicates that have popped up over the last week)