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(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

wants to merge 1 commit into from

Conversation

bartektelec
Copy link
Contributor

resolves #1655

@codesandbox-ci
Copy link

codesandbox-ci bot commented Jul 6, 2023

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 387bee8:

Sandbox Source
MSW React Configuration

@kettanaito
Copy link
Member

That's quite a huge diff. @bartektelec could you please check what version of pnpm you're using? 👀

@kettanaito
Copy link
Member

It seems that you are using pnpm different from the one that generated the lockfile on main (side note: would be nice to add pnpm's version to engines on my side). You can see that in the failed CI job:

https://github.com/mswjs/msw/actions/runs/5667678658/job/15358726533?pr=1656

@bartektelec, could you please make sure you're using pnpm@7.12, run pnpm install again and commit the changes? That should unblock the workflow. Thanks.

@bartektelec
Copy link
Contributor Author

Sorry for the inconvenience, setting packageManager field in package.json could be a good idea as well.

Pushed an updated version that I generated with pnpm@7.12 should be fine now.

@kettanaito
Copy link
Member

Thank you, @bartektelec! Would you be open to setting the packageManager in package.json as a follow-up pull request? I think it's a great idea, it can improve the contributing experience drastically. We can also list pnpm in engines, requiring contributors to have pnpm of a specific version range installed on their machine.

@@ -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 vite@4.2.0 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?

@kettanaito
Copy link
Member

Released: v1.3.2 🎉

This has been released in v1.3.2!

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.

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

Successfully merging this pull request may close these issues.

GraphQL version >=16.7.0 throws exceptions about Unexpected string
2 participants