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: makes the library esm-compatible #1399

Merged
merged 6 commits into from Nov 7, 2022

Conversation

ivanhofer
Copy link
Contributor

@ivanhofer ivanhofer commented Sep 11, 2022

fixes #1267

I have investigated #1267 a bit. There are a few road blocks to make msw/node work in a .mjs file:

@codesandbox-ci
Copy link

codesandbox-ci bot commented Sep 11, 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 2755f43:

Sandbox Source
MSW React Configuration

src/context/status.ts Outdated Show resolved Hide resolved
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'
Copy link
Contributor

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.

Copy link
Member

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.

Copy link
Contributor

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)

@mattcosta7
Copy link
Contributor

dynamic import of node-fetch

I think we could get around this with something like

useFetch = isNodeProcess() ? (input, init) => import('node-fetch').then(({default: nodeFetch}) => fetch(input, init)) : window.fetch

I'm curious if we could also avoid using node-fetch directly in this library and just stipulate that globalThis.fetch be defined already (via a user polyfill) instead?

@ivanhofer ivanhofer marked this pull request as ready for review September 13, 2022 16:15
@ivanhofer
Copy link
Contributor Author

With this branch it is now poosible to run the following esm file:

test.mjs

import { setupServer } from 'msw/node'

setupServer()

with the command

node ./test.mjs

mizdra added a commit to mizdra/happy-css-modules that referenced this pull request Sep 18, 2022
mizdra added a commit to mizdra/happy-css-modules that referenced this pull request Sep 18, 2022
@filipw01
Copy link

Nice, work. I was about to open an issue, but it looks like it should be fixed once this PR is merged 😄

@ivanhofer
Copy link
Contributor Author

I think this PR is ready to be merged. Or is something still missing?

@mataspetrikas
Copy link

@kettanaito @ivanhofer is this coming into the next release?

Copy link
Member

@kettanaito kettanaito left a 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.

@kettanaito
Copy link
Member

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

@kettanaito kettanaito changed the title feat: better esm support feat: makes the library esm-compatible Nov 5, 2022
@kettanaito kettanaito merged commit 41798eb into mswjs:main Nov 7, 2022
@kettanaito
Copy link
Member

Released: v0.48.0 🎉

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

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.

Can't resolve '@mswjs/interceptors/lib/interceptors/ClientRequest'
5 participants