-
-
Notifications
You must be signed in to change notification settings - Fork 495
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: makes the library esm-compatible #1399
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 2755f43:
|
import { ClientRequestInterceptor } from '@mswjs/interceptors/lib/interceptors/ClientRequest' | ||
import { XMLHttpRequestInterceptor } from '@mswjs/interceptors/lib/interceptors/XMLHttpRequest' | ||
import { ClientRequestInterceptor } from '@mswjs/interceptors/lib/interceptors/ClientRequest/index.js' | ||
import { XMLHttpRequestInterceptor } from '@mswjs/interceptors/lib/interceptors/XMLHttpRequest/index.js' |
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.
we should really just update interceptors to expose an esm build.
I started on that over the weekend, and have some thoughts on how we could do that, but I think we'll need to probably remove debug
as a dependency there - since it relies on tty
and os
, and i'm having issues getting those properly bundled to a browser compat version, that multiple build tools like.
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.
We need to look into some debug
recipes perhaps. It has browser support, so it should run in the browser. Perhaps we can do some import map for this dependency to ensure that in the browser it always imports the browser dist.
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.
Yeah, it mostly relates to the fact that debug
doesn't have an esm build, so loading in a browser directly from @web/dev-server
seems problematic.
Maybe that just means users in those environments have to do some extra setup, and that's enough. Will keep playing with configurations there, because it's also possible we don't need to be as explicit there.
I was testing using tsup to bundle, however using tsc to generate cjs and esm transpilations might help avoid issues related to that (since it won't try to handle debug directly)
I think we could get around this with something like
I'm curious if we could also avoid using node-fetch directly in this library and just stipulate that |
With this branch it is now poosible to run the following test.mjs import { setupServer } from 'msw/node'
setupServer() with the command
|
47b7bac
to
818a27b
Compare
Nice, work. I was about to open an issue, but it looks like it should be fixed once this PR is merged 😄 |
I think this PR is ready to be merged. Or is something still missing? |
@kettanaito @ivanhofer is this coming into the next release? |
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.
Finally got to this improvement. Looks superb 🎉 Updated against the latest main, waiting for the CI before marking it as a release candidate. Expect to land in the next minor release.
Thank you for improving MSW, @ivanhofer! If you ever feel like exercising your skill again, we could also migrate the interceptors library to ESM.
The only thing I'd add is an automated test for the distributed ESM format. Let me see if I can dig out the previous test I've written... |
Released: v0.48.0 🎉This has been released in v0.48.0! Make sure to always update to the latest version ( Predictable release automation by @ossjs/release. |
fixes #1267
I have investigated #1267 a bit. There are a few road blocks to make
msw/node
work in a.mjs
file:lib/index.js
instead of'lib'
in a few files (easy to achive)tsup
does not support import assertions (needed here https://github.com/mswjs/msw/blob/main/src/context/status.ts#L1) import assertion get removed from output egoist/tsup#714workaround: feat: makes the library esm-compatible #1399 (comment)
dynamic import ofnode-fetch
(https://github.com/mswjs/msw/blob/main/src/context/fetch.ts#L6) is not supported in esm context. How to fix "Dynamic require of "os" is not supported" evanw/esbuild#1921workaround: feat: makes the library esm-compatible #1399 (comment)