-
-
Notifications
You must be signed in to change notification settings - Fork 529
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: inline statuses
dependency during the build
#1458
Conversation
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 d5dffb8:
|
@@ -102,7 +102,6 @@ | |||
"node-fetch": "^2.6.7", | |||
"outvariant": "^1.3.0", | |||
"path-to-regexp": "^6.2.0", | |||
"statuses": "^2.0.0", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
removing it from dependencies
it's already in devDependencies, and if it's not in dependencies
then tsup will inline it at build-time
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Huh, that's funny how it was in both deps and dev deps. A good find.
Hey, @mattcosta7. Thanks for addressing this! 👏
Didn't know about this behavior. Sounds like what we need here. On a somewhat related note, can we create an automated test for MSW compatibility with ESM? I used to have one, where we bundle the library into ESM format and then open it in the browser to see if there are any runtime exceptions. This wasn't ideal but it gave some minimal coverage to be sure that, at least, it will get parsed without issues. Do you think we can create a test like this at the current state? I've noticed we have ESM build target only for Node and Native builds so maybe what I'm saying here doesn't make sense (feel free to point that out). |
Is there any help needed to move this PR into release? |
statuses
as devDepstatuses
dependency during the build
@thebergamo, I will merge this today not to block everyone but seeing how often we face ESM-related issues, I'd be great to have an automated way to assert this kind of fixes. |
I agree with you, as of now, to be honest I tried most of the previous versions of MSW and couldn't find one that worked. |
That will absolutely keep happening because MSW doesn't have full ESM support. We are looking for more contributors to make this happen. |
I can try to look into a test case for this when I get some time! I think we can do something where we just import in node, but i haven't looked into that before, and not sure what we'd need to setup to do that (to make sure it imports the esm version) |
If you can get a test case setup that would be very helpful in tracking down the other areas we need to support, as well as avoid regression once fixed! |
One other thing here - I only validated that this inlines the json object, instead of trying to |
Haven't played also with ESM, but I can try to help and learn in the process:) |
Anyway... using directly master with PNPM link worked for me, so no issue with MSW at all using Next.js and typescript. |
Yeah, I think using Node example would be the most performant way of such a test. We used to run ESM in a browser but in hindsight that was unnecessary. |
Released: v0.48.3 🎉This has been released in v0.48.3! Make sure to always update to the latest version ( Predictable release automation by @ossjs/release. |
fixes #1455
by only including statuses as a dev dependency, tsup will inline it instead of importing it