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: make GraphQL a peer dependency, support GraphQL v15.0 #1356

Merged
merged 2 commits into from
Aug 21, 2022

Conversation

JohnDaly
Copy link
Contributor

@JohnDaly JohnDaly commented Aug 1, 2022

The graphql package throws errors if it's not treated as a singleton dependency. It's recommended for libraries to install it as a peer dependency (see: graphql/graphql-js#594 (comment)).

To allow MSW to work with projects that are using graphql v15, I've made a few small changes to the code to avoid using OperationTypeNode as an enum. The updated code is compatible with both v15 and v16 of the graphql package.

@codesandbox-ci
Copy link

codesandbox-ci bot commented Aug 1, 2022

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 7a8482a:

Sandbox Source
MSW React Configuration

@JohnDaly
Copy link
Contributor Author

JohnDaly commented Aug 2, 2022

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 ebe786c:

Sandbox Source
MSW React Configuration

The Sandbox template will need to be updated to include graphql in its list of dependencies

@kettanaito
Copy link
Member

I will update the sandbox once this is merged.

The 'graphql' code that is needed at runtime will be dynamically loaded. Existing static imports of
'graphql' are designated as type imports.
@JohnDaly
Copy link
Contributor Author

@kettanaito have you had a chance to review the latest changes?

@@ -116,6 +115,7 @@
"eslint-plugin-prettier": "^3.4.0",
"fs-extra": "^10.0.0",
"fs-teardown": "^0.3.0",
"graphql": "^16.3.0",
Copy link
Member

Choose a reason for hiding this comment

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

Is it safe to migrate to v16 given the optional range is 15/16? Are there any features that are dropped in v16 that we may accidentally rely upon?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Since v16 is the version that MSW has used in the latest release, I think it makes sense to keep it as the devDependency version.

There are two main changes in this PR that affect run-time compatibility:

  1. Changing usage of OperationTypeNode from enum to string
  2. The parse function

Changing OperationTypeNode should be safe to do, the enum in v16 maps to the same strings that were used in v15.

The parse function from graphql is the only piece of runtime code that is actually used by MSW. It's being used to get the OperationDefinitionNode for the GraphQL operation that is intercepted. From what I can tell, the fields that MSW is interested in on OperationDefinitionNode are unchanged between v15 and v16.

Based on this, I think it should be safe to list v15 as a dependency.

It's also worth noting that v15 is, at the time of writing, the most commonly downloaded version of graphql on NPM (3+ million downloads a week, compared to ~2 million for v16). It would be good to be backwards compatible with v15, because lots of MSW users probably have that version in their projects.

@kettanaito
Copy link
Member

Hey, @JohnDaly. I've finally got to review the changes you've made and they are great!

I've left two questions:

  • Whether the compiled sandbox is a confirmation that we are not breaking apps for people now;
  • Is it safe to rely on GraphQL v16 internally given we permit v15-16 as optional dependency?

@JohnDaly
Copy link
Contributor Author

Hey, @JohnDaly. I've finally got to review the changes you've made and they are great!

I've left two questions:

  • Whether the compiled sandbox is a confirmation that we are not breaking apps for people now;
  • Is it safe to rely on GraphQL v16 internally given we permit v15-16 as optional dependency?

@kettanaito Let me know if you have any follow-up questions on this

@kettanaito kettanaito changed the title feat: make 'graphql' a peer dependency. Add support for 'graphql' v15 feat: make GraphQL a peer dependency, support GraphQL v15.0 Aug 21, 2022
@kettanaito kettanaito merged commit ca0e2e0 into mswjs:main Aug 21, 2022
@kettanaito
Copy link
Member

Thank you for the superb improvement, @JohnDaly! 🎉 Feel free to tackle any other tasks as long as they look interesting to you. I know I could use your help. Thanks.

@kettanaito
Copy link
Member

Released: v0.45.0 🎉

This has been released in v0.45.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.

@limonte
Copy link

limonte commented Aug 22, 2022

This is the misuse of peer dependencies. Please read the docs about peer deps and revert this change as the testing helper shouldn't require projects to install graphql https://nodejs.org/en/blog/npm/peer-dependencies/#the-solution-peer-dependencies

What we need is a way of expressing these "dependencies" between plugins and their host package. Some way of saying, "I only work when plugged in to version 1.2.x of my host package, so if you install me, be sure that it's alongside a compatible host." We call this relationship a peer dependency.

msw will most certainly work without graphql installed, so graphql is definitely not a peer dependency.

@kleinfreund
Copy link
Contributor

As an example for what now happens in a project that doesn’t use/need graphql during build:

Module not found: Error: Can't resolve 'graphql' in 'node_modules/msw/lib'

@jljouannic
Copy link

Another example, here is what tsc command gives me now in a project in which I do not need graphql:

node_modules/msw/lib/index.d.ts:4:63 - error TS2307: Cannot find module 'graphql' or its corresponding type declarations.

4 import { GraphQLError, OperationTypeNode, DocumentNode } from 'graphql';
                                                                ~~~~~~~~~
Found 1 error in node_modules/msw/lib/index.d.ts:4

@limonte
Copy link

limonte commented Aug 22, 2022

To clarify my previous comment. What this PR and graphql as peer dependency means is:

starting from 0.45.0, msw will not work without graphql dependency

IMO It doesn't make sense to require msw users to have graphql as a dependency for their projects.

@kettanaito
Copy link
Member

Thanks for your concern, @limonte. What is the correct way to express the MSW-graphql dependency then? Because I think NPM, as a package manager, severely lacks in the means of expressing this kind of dependencies.

To be absolutely clear: graphql is an optional peer dependency of MSW:

  • Optional, because graphql is not required for MSW to execute;
  • Peer, because we have no intention of bundling graphql as a part of MSW. That is also a bad idea since GraphQL versions mismatches would be horrible.

If anyone has a better way to express this relationship, the pull requests are open and your contribution would be highly valued.

As to the compilation issues, those happen because graphql is not correctly isolated from the rest of the package (just as I've suspected). It's either lacking proper tree-shacking or TypeScript configuration so that it doesn't attempt to load it. For instance, this line of code is entirely incorrect:

import { GraphQLError, OperationTypeNode, DocumentNode } from 'graphql';

We can no longer import things from graphql directly. Moreover, that entire import should be import type because we rely exclusively on parse from graphql and nothing else for runtime.

@kettanaito
Copy link
Member

IMO It doesn't make sense to require msw users to have graphql as a dependency for their projects.

This was never the intention. We've spent a lot of time testing this but this still slipped through. I'd appreciate if everyone stopped treating this as a deliberate shortcoming and expressed an ounce of compassion for people trying to make this package better. Thanks.

@kettanaito
Copy link
Member

I'm not going to revert anything. 0.45.0 only introduces the graphql change (and thus was correctly released as a breaking change), so you're not missing on anything else. Let this be an opportunity for people to contribute instead of complain. This needs a fix, not a revert. I will be more than glad to review and merge the fix within a day if anyone actually decides to tackle this.

@limonte
Copy link

limonte commented Aug 22, 2022

Thank you for the replies @kettanaito 🙏

I won't be able to contribute, but IMO graphql API should be separated from the core package and imported like this:

import { graphql } from 'msw/graphql'

similar to

import { setupServer } from 'msw/node'

In this case the graphql dependency can be moved to devDependencies so tests will work.

And from the users' perspective graphql types will be imported from whatever version they have installed in their project.

@kettanaito
Copy link
Member

And from the users' perspective graphql types will be imported from whatever version they have installed in their project.

That's the issue though. We need to specify the supported range for GraphQL you can use with MSW. We can't guarantee that new features in GraphQL 17 won't break MSW (imagine GraphQL renames the parse function).

Moving the GraphQL to msw/graphql is interesting. I don't know if we can bundle things like that straight away. There's msw/node because it's, basically, a separate library. msw/graphql would be an entire copy of msw plus graphql. Your bundles would get huge (not really a concern since it's a devtool but still).

@kettanaito
Copy link
Member

And, once again, the issue is actually with bundling and TypeScript, not NPM. And so the solution should likely be in those areas. But, as I've said, I have no interest in working on this, so it's up to everyone to contribute with ideas and fixes.

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.

6 participants