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.

Sign up for GitHub

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(deps): lock graphql dependency to prevent unstable versions #1656

Closed
wants to merge 1 commit into from
Closed

fix(deps): lock graphql dependency to prevent unstable versions #1656

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -95,7 +95,7 @@
"chalk": "4.1.1",
"chokidar": "^3.4.2",
"cookie": "^0.4.2",
"graphql": "^15.0.0 || ^16.0.0",
"graphql": "^15.0.0 || >=16.0.0 <16.7.0",
Copy link
Member

Choose a reason for hiding this comment

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

I think we should remove the upper range (<16.7.0). I believe you added it because GraphQL used to have a broken 16.x release. They don't have that anymore, so we should really set this to:

"graphql": "^15.0.0 || ^16.7.0"

What do you think about this? @bartektelec

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This may be tricky. The 16.7.x versions are exactly what break the build, both 16.7.0 and 16.7.1 introduce this problem, but as you mentioned this issue is bound to the compiler. I did some digging and seems like there are still problems with these versions and other packages are affected - as seen here graphql/graphql-js#3928

In fact the issue they link in the comment right where the code breaks is still open: graphql/graphql-js#2317

I also checked out the codesandbox link I provided in the issue and it still breaks, BUT - only if using vite@^3.0.0 when switched to vite@^4.0.0 the problem gets solved. Further narrowed it down to [email protected] being the last version that keeps breaking.

So I'm not sure where we should take it from here, there are tons of projects that still run of older vite and rollup versions and will probably do so for a while

Completely agree this should be fixed by graphql-js but until it is I would advice locking the version of graphql to lower than 16.7.0 otherwise it will affect all MSW's users that depend on vite@<=4.2.0 or whatever rollup version that breaks.

Walk around for the end user is either updating the bundler if possible or doing the nasty thing of locking the graphql version manually in the lock file

Copy link
Member

Choose a reason for hiding this comment

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

If that's the case and the issue in graphql still persists, then I think the right course of action here is to do nothing.

By locking the upper range of the GraphQL version we are making a decision on the user's behalf. With that, comes quite a share of responsibility, as we now should lift off that range restriction as soon as the issue is fixed in the right third-party package. Suddenly, MSW itself becomes bound to the state of the issue, while it shouldn't be.

I see it this way:

  • If you aren't using GraphQL, you are not affected in any way.
  • If you are using GraphQL in your app but only in Node.js, you are not affected in any way.
  • If you are using GraphQL in your app for the browser, you get an error from your compiler anyway. This has nothing to do with MSW, and you will get this error even if you don't have MSW in your project.

The debugging journey for the users that encounter this issue now becomes:

  1. I notice the issue.
  2. I research the issue.
  3. I find the recommendation on the graphql side to pin my graphql package version to a specific version that doesn't have the issue.
  4. I keep working.

Originally, I thought the issue has already been fixed, and the intention was to ensure that MSW users are encouraged to migrate to more recent versions of graphql. But if it's the opposite, and the intention is to lock in that dependency version, I'd vote against it.

Let me know if I understood the current state of things correctly. Always open to discussion.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is partially correct, although even if you are not using graphql in your project you are still affected.
Like in the codesandbox example I provided you don't even need to have a single request or MSW handler for it to crash.

So yes - if you are using an older version of vite/rollup that doesn't compile the problematic code properly you will need to manually pin the peer dependency of MSW to be a lower version of graphql.

Copy link
Member

Choose a reason for hiding this comment

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

I don't deny that. All I mean is that MSW isn't the right place to propagate this kind of error, as it's much broader and affects more things than MSW. Do you still have some concerns about it?

"headers-polyfill": "^3.1.2",
"inquirer": "^8.2.0",
"is-node-process": "^1.2.0",
Expand Down
2 changes: 1 addition & 1 deletion pnpm-lock.yaml
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.