feat: distribute the library for browser and node in ESM and CJS #313

cdimitroulas · 2022-11-30T13:53:08Z

Bundle the library, including remix-run and other polyfill libs, so that consumers of the lib can consume it without having to bundle/transpile it themselves.

tsup.config.ts

cdimitroulas · 2022-11-30T16:44:51Z

OK I switched to using multiple entrypoints in the tsup config and this looks more like what we want.

Here is the structure of the outputted files:

The interceptors import from the chunks which are all the shared bits of code

mattcosta7 · 2022-11-30T17:02:15Z

OK I switched to using multiple entrypoints in the tsup config and this looks more like what we want.

Here is the structure of the outputted files:

The interceptors import from the chunks which are all the shared bits of code

Nice! yeah, that's more like what I think we're looking for!

kettanaito · 2022-12-01T12:22:42Z

The interceptors import from the chunks which are all the shared bits of code

@cdimitroulas, This is fantastic!

While we're at it, folks, what is your opinion on #52? I think it would be great to be able to import interceptors directly from the root of the package like so:

import { ClientRequestInterecptor } from '@mswjs/interceptors/ClientRequestInterceptor'

This is possible via exports in package.json but I don't know if that will be universally supported by all consumers (CJS/ESM). This can also be done entirely separately from this change, I'm just asking for your opinion on this.

package.json

tsup.config.ts

mattcosta7 · 2022-12-01T12:50:12Z

The interceptors import from the chunks which are all the shared bits of code

@cdimitroulas, This is fantastic!

While we're at it, folks, what is your opinion on #52? I think it would be great to be able to import interceptors directly from the root of the package like so:
import { ClientRequestInterecptor } from '@mswjs/interceptors/ClientRequestInterceptor'
This is possible via exports in package.json but I don't know if that will be universally supported by all consumers (CJS/ESM). This can also be done entirely separately from this change, I'm just asking for your opinion on this.

I think this should work everywhere that package.json exports work.

We may need to add fake folders/package.json for older versions of typescript and node to handle, but otherwise i think this is the path we should be on here.

we should add a block like this to the package.json (once we configure both esm and cjs builds from tsup)

"exports": {
    ".": {
        "types": "...path to index.d.ts for root package",
        "require": "...path to cjs root pacakge",
        "default": "...path to esm root package"
    },
    "./ClientRequest": {
       ...same format as above
    },
    "./XMLHttpRequest": {
       ...same format as above
    },
    "./fetch": {
       ...same format as above
    }
}

then we'll probably also need a folder for each of these (empty, just with a package.json)
ClientRequestIntercepot, XMLHTTPRequest and fetch with the main, types, and module fields corresponding to the same data in the exports blocks.

cdimitroulas · 2022-12-02T11:37:32Z

@mattcosta7 I've added the "exports" field to package.json, please take a look and maybe also test out the build locally to see if it matches your expectations.

I think it would be great to be able to import interceptors directly from the root of the package like so:

import { ClientRequestInterecptor } from '@mswjs/interceptors/ClientRequestInterceptor'

I think this should work fine based on the current build output (yarn build:new for now, I'll overwrite yarn build when we're ready to finalize this PR)

cdimitroulas · 2022-12-06T11:12:12Z

@mattcosta7 @kettanaito when you have a moment I would appreciate if we could finalize some of these discussions so I can finalize the code and mark the PR as ready for review

kettanaito · 2022-12-06T12:17:40Z

package.json

+      "import": "./lib/interceptors/ClientRequest/index.mjs",
+      "types": "./lib/interceptors/ClientRequest/index.d.ts"
+    },
+    "./lib/interceptors/fetch": {


Should we drop ./lib/interceptors/ from the exports paths? So that you could import interceptors like so:

import X from '@mswjsw/interceptors/ClientRequest'

The more we work on this, the more I think it needs to be a separate change. I wouldn't want to ask you to restructure all of your work, so if you feel it's already under substantial progress, we can ship this together with the remix-run change. If not, I think we're safer releasing the exports changes separately.

I've moved it to a separate PR. Let's tackle that once we get the new bundling merged since I'm not sure if it depends on this work or not.
I've opened a draft PR on my fork just to keep a record of the work: cdimitroulas#1

I don't think these changes are dependent. Thanks for moving it away 👍

kettanaito · 2022-12-06T12:20:14Z

Hey, @cdimitroulas. I was having a couple of busy weeks at work. I've updated all the discussions, summarizing them below:

Move headers-polyfill to dependencies as it ships both as CJS and ESM so it's safe to depend on it as-is (no need to bundle it).
Consider moving the exports changes to a separate pull request. I leave this to you and whether you feel comfortable extracting those changes. If not, we can ship it as a part of this change as well, just going to bump a minor version.

I wouldn't want to introduce empty placeholder directories like @mattcosta7 mentions but I think we need that for full compatibility. Do you know if there's a support map for exports in package.json or something? I believe all modern bundlers and the latest Node respects that field but I wonder how far the support goes in terms of previous versions.

re mswjs#283

cdimitroulas · 2022-12-06T12:46:53Z

Do you know if there's a support map for exports in package.json or something? I believe all modern bundlers and the latest Node respects that field but I wonder how far the support goes in terms of previous versions.

I'm not sure such a thing exists, I did find this helpful SO answer which is relevant:
https://stackoverflow.com/a/74551879/7133623

Not sure about bundler support but I imagine it's as you say and they all support it (at least on more recent versions)

kettanaito · 2022-12-07T13:44:30Z

I've added RemoteHttpInterceptor as the entrypoint so it can be imported.

Now, seems like all browser tests are failing because some Node code ends up in lib/index.js:

    BREAKING CHANGE: webpack < 5 used to include polyfills for node.js core modules by default.
    This is no longer the case. Verify if you need this module and configure a polyfill for it.

    If you want to include a polyfill, you need to:
        - add a fallback 'resolve.fallback: { \"http\": require.resolve(\"stream-http\") }'
        - install 'stream-http'
    If you don't want to include a polyfill, you can use an empty module like this:
        resolve.fallback: { \"http\": false }",
        "moduleId": "./lib/chunk-EYK25V6X.js",
        "moduleIdentifier": "/mswjs/interceptors/lib/chunk-EYK25V6X.js",
        "moduleName": "./lib/chunk-EYK25V6X.js",
        "moduleTrace": Array [
          [Object],
          [Object],
        ],
        "stack": "ModuleNotFoundError: Module not found: Error: Can't resolve 'http' in '/mswjs/interceptors/lib'

@cdimitroulas, do you have some suspicion as to why this happens?

cdimitroulas · 2022-12-07T15:54:29Z

No idea, I'll try to figure it out

cdimitroulas · 2022-12-07T16:12:38Z

It's not all the browser tests that are failing, the fetch ones are passing. Only the XMLHttpRequest ones are failing.

It seems that a chunk that is split off by the code-splitting that is used by both the XMLHttpRequest interceptor and the ClientRequest interceptor has this in it:

// node_modules/@remix-run/web-fetch/src/headers.js
var _util = require('util');
var _http = require('http'); var _http2 = _interopRequireDefault(_http);

This suggests that it's somehow related to remix-run/web-fetch :/

cdimitroulas · 2022-12-07T16:48:04Z

It seems remix is configured to automatically deal with this stuff... https://remix.run/docs/en/v1/guides/constraints#server-code-pruning

mattcosta7 · 2022-12-07T17:04:07Z

It's not all the browser tests that are failing, the fetch ones are passing. Only the XMLHttpRequest ones are failing.

It seems that a chunk that is split off by the code-splitting that is used by both the XMLHttpRequest interceptor and the ClientRequest interceptor has this in it:
// node_modules/@remix-run/web-fetch/src/headers.js
var _util = require('util');
var _http = require('http'); var _http2 = _interopRequireDefault(_http);
This suggests that it's somehow related to remix-run/web-fetch :/

OOof hmm. I think since we're bundling this with a node target, we're probably only getting the node imports for remix-run/fetch?

I wonder if we actually need a 'browser' build that's separate from the node build, which could potentially be a separate tsup config? the docs specificy - https://tsup.egoist.dev/#target-environment that target environment is node14 by default? do we need one that targets browsers to get the browser build of that library? (just guessing)

cdimitroulas · 2022-12-07T17:16:24Z

I think the correct flag is esbuild's platform which can be set to node or browser. Unfortunately, trying to build with platform: "browser" just results in the same errors but at build-time instead:

ESM Build start
✘ [ERROR] Could not resolve "http"

    src/interceptors/ClientRequest/index.ts:1:17:
      1 │ import http from 'http'
        ╵                  ~~~~~~

  The package "http" wasn't found on the file system but is built into node. Are you trying to
  bundle for node? You can use "platform: 'node'" to do that, which will remove this error.

✘ [ERROR] Could not resolve "http"

    src/interceptors/ClientRequest/index.ts:1:17:
      1 │ import http from 'http'
        ╵                  ~~~~~~

  The package "http" wasn't found on the file system but is built into node. Are you trying to
  bundle for node? You can use "platform: 'node'" to do that, which will remove this error.

✘ [ERROR] Could not resolve "https"

    src/interceptors/ClientRequest/index.ts:2:18:
      2 │ import https from 'https'
        ╵                   ~~~~~~~

  The package "https" wasn't found on the file system but is built into node. Are you trying to
  bundle for node? You can use "platform: 'node'" to do that, which will remove this error.

...etc.

cdimitroulas · 2022-12-07T17:17:24Z

I'm going to see if there's a way to exclude remix-run/web-fetch in browser builds since theoretically it shouldn't be needed, right? Everything used from there like Response are browser globals

cdimitroulas · 2022-12-07T17:33:55Z

I was able to get the tests to pass, but I'm not sure how the build output should look like when building for node/browser separately.
For now, I just arbitrarily created lib/node/ and lib/browser/ and updated the imports for the tests:

Here's a gist showing the changes I made as I don't feel like this is something I want to commit here:
https://gist.github.com/cdimitroulas/caf930091aa7c63fc696e6521ade465a

cdimitroulas · 2023-01-23T12:44:28Z

@mattcosta7 I reordered the keys in the exports as per your suggestion

kettanaito · 2023-01-23T20:35:53Z

@mattcosta7, thanks for the review! And @cdimitroulas, thank you so much for addressing those points so quickly! I've rebased this branch, and solved the package.json conflict. Will continue with testing in MSW, let's see if the order of those fields solved the issue for the IIFE build.

cdimitroulas · 2023-01-31T16:48:54Z

@kettanaito did the latest changes fix the problems you saw when testing in MSW?

kettanaito · 2023-02-01T00:41:16Z

Hey, @cdimitroulas. I haven't tested that yet. Planning on returning to open source this/next week, need to arrange a few other things.

The changes to Interceptors look good from what I can gather. The order sensitivity that @mattcosta7 mentioned was a great addition to this, now to see what exactly gets bundled in when tsup builds into IIFE. For some reason, I suspect it doesn't respect the exports field despite the platform being the "browser". I'd expect it to respect that.

kettanaito · 2023-02-01T00:43:35Z

The IIFE integration test passes in MSW with these changes to Interceptors linked:

 PASS  test/msw-api/distribution/iife.test.ts
  ✓ supports the usage of the iife bundle in a <script> tag (1169 ms)

🎉

kettanaito · 2023-02-01T00:44:31Z

There are, however, failing unit tests because somewhere in Interceptors we rely on TextEncoder, which Jest just bluntly ignores (Jest is notoriously ~~bad~~ strict with ignoring accepted Node.js standard API).

 FAIL  src/utils/logging/serializeRequest.test.ts
  ● Test suite failed to run

    ReferenceError: TextEncoder is not defined

      1 | "use strict";Object.defineProperty(exports, "__esModule", {value: true});// node_modules/web-encoding/src/lib.mjs
    > 2 | var Encoder = TextEncoder;
        |               ^
      3 | var Decoder = TextDecoder;

This happens because web-encoding—the library that we import TextEncoder from—optimistically uses the global TextEncoder API for its Node.js target:

lib.js is browser-oriented (ambiguous) module.
lib.mjs is Node-oriented module.

  "main": "./src/lib.js",
  "module": "./src/lib.mjs",
  "browser": "./src/lib.js",
  "types": "./src/lib.d.ts",
  "exports": {
    ".": {
      "import": "./src/lib.mjs",
      "require": "./src/lib.js"
    }
  },

// src/lib.mjs
// In node `export { TextEncoder }` throws:
// "Export 'TextEncoder' is not defined in module"
// To workaround we first define constants and then export with as.
const Encoder = TextEncoder
const Decoder = TextDecoder

export { Encoder as TextEncoder, Decoder as TextDecoder }

Those globals won't be defined in Jest. They are defined in Node.js.

This may have something to do with how we require/bundle web-encoding. It would be nice to force it to use lib.js that is ambiguous and should work in Jest:

// lib.js
"use strict"

exports.TextEncoder =
  typeof TextEncoder !== "undefined" ? TextEncoder : require("util").TextEncoder

exports.TextDecoder =
  typeof TextDecoder !== "undefined" ? TextDecoder : require("util").TextDecoder

This will work because it will use require('util').TextEncoder in Jest, since TextEncoder doesn't exist there.

kettanaito · 2023-02-01T00:55:51Z

Solution to `web-encoding`

I just added a manual shim to Interceptors to provide ambiguous export for environments like Jest.

kettanaito · 2023-02-01T01:00:06Z

Issue: Jest doesn't understand the `exports` field

Looks like Jest doesn't handle exports correctly in its resolver:

 FAIL  test/msw-api/setup-server/life-cycle-events/on.test.ts
  ● Test suite failed to run

    Cannot find module '@mswjs/interceptors/ClientRequest' from '../lib/node/index.js'

    Require stack:
      /Users/kettanaito/Projects/mswjs/msw/lib/node/index.js
      msw-api/setup-server/life-cycle-events/on.test.ts

    > 1 | import { ClientRequestInterceptor } from '@mswjs/interceptors/ClientRequest'
        |                                          ^
      2 | import { XMLHttpRequestInterceptor } from '@mswjs/interceptors/XMLHttpRequest'
      3 | import { RequestHandler } from '../handlers/RequestHandler'
      4 | import { SetupServerApi } from './SetupServerApi'

      at Resolver.resolveModule (../node_modules/jest-resolve/build/index.js:306:11)
      at Object.<anonymous> (../src/node/setupServer.ts:1:42)

See Support package exports in jest-resolve jestjs/jest#9771

The issue appears to be fixed in Jest@28 (27 too?). But jumping two major versions of Jest foreshadows a traumatic experience. I will try that to prove that MSW tests pass with this Interceptors change.

cdimitroulas · 2023-02-01T18:49:37Z

Jumping from Jest v26 -> v27 doesn't seem too bad https://jestjs.io/blog/2021/05/25/jest-27#features-coming-with-breaking-changes

Jumping from v27 -> v28 requires working through this guide though

kettanaito · 2023-02-01T19:10:14Z

I wonder if Jest 27 brings in ESM support... The references I find mention Jest 28 if not 29.

All and all, this looks like an endless loop of testing setup update that I've been postponing for forever. I don't mind doing it in MSW's repo, it just takes time. Everything does.

Working on that mswjs/msw#1394

kettanaito · 2023-02-01T19:42:27Z

I wonder if we just used undici in the Fetch API branch, and removed remix-run dependencies that force us to bundle everything as ESM, would that be less changes all around the stack? 🤔

mattcosta7 · 2023-02-01T21:16:13Z

I wonder if we just used undici in the Fetch API branch, and removed remix-run dependencies that force us to bundle everything as ESM, would that be less changes all around the stack? 🤔

offhand, i'm not sure of node support, would we need to lose backward support? (ie, would we only support versions where unidici already exists? and referenceable on globalThis?)

the less we need to bundle, the better I think - so if so, it sounds worth exploring

kettanaito · 2023-02-06T16:15:03Z

@mattcosta7,

would we need to lose backward support? (ie, would we only support versions where unidici already exists? and referenceable on globalThis?)

Eventually, yes! That's the plan for this autumn when Node 16 goes to pass and we only have the versions of Node that ship global fetch throughout the board. Until then, we need to resort to some polyfill to support Node 16.

kettanaito · 2023-02-06T16:21:56Z

Trying this with the Fetch API branch

✅ Build
✅ Unit tests
❌ Integration tests

 FAIL  test/msw-api/setup-server/scenarios/response-patching.test.ts
  ● Test suite failed to run

    Cannot find module '@mswjs/interceptors/ClientRequest' from '../lib/node/index.js'

    Require stack:
      /mswjs/msw/lib/node/index.js
      msw-api/setup-server/scenarios/response-patching.test.ts

    > 1 | import { ClientRequestInterceptor } from '@mswjs/interceptors/ClientRequest'

All the errors seem to have the same origin: they fail to resolve the "exports" to the interceptors.

This is likely caused by Jest@26 missing support for "exports". But since we provide backward-compatibility with the ./ClientRequest/package.json paths, I'd expect older Jest to work. Hm.

kettanaito

@cdimitroulas @mattcosta7 thank you so much for all the work on this. I think the changes are good to go and I'm going to merge them once the CI passes. I still cannot fully test them in MSW due to the old Jest being used there but I've decided to slowly make our way towards that, as updating its test dependencies is a time-consuming endeavor and I don't wish to stall this change any longer.

kettanaito · 2023-02-18T23:20:05Z

Released: v0.21.0 🎉

This has been released in v0.21.0!

📄 Release notes
📦 npm package

Make sure to always update to the latest version (npm i @mswjs/interceptors@latest) to get the newest features and bug fixes.

Predictable release automation by @ossjs/release.

blaenk · 2023-02-20T06:50:59Z

Is it just me, or does it appear that this change made the presets unreachable now? This sample code no longer works for importing the browserInterceptors preset:

import { BatchInterceptor } from '@mswjs/interceptors'
import browserInterceptors from '@mswjs/interceptors/lib/presets/browser'

const interceptor = new BatchInterceptor({
  name: 'my-interceptor',
  interceptors: browserInterceptors,
})
interceptor.on('request', listener)

cdimitroulas · 2023-02-20T13:10:32Z

Most probably yes @blaenk - we'll need to add these exports explicitly at the top-level somewhere I think and update the docs to reflect this. Do you have capacity to open a PR on that?

blaenk · 2023-02-21T01:19:17Z

Sorry, I don't have the bandwidth at the moment. For now I pinned to the previous release.

cdimitroulas mentioned this pull request Nov 30, 2022

feat: support Node.js 18 #283

Closed

mattcosta7 reviewed Nov 30, 2022

View reviewed changes

tsup.config.ts Outdated Show resolved Hide resolved

mattcosta7 reviewed Nov 30, 2022

View reviewed changes

tsup.config.ts Outdated Show resolved Hide resolved

cdimitroulas force-pushed the feat/bundle-the-library branch from fb23783 to 083dcc2 Compare November 30, 2022 16:43

kettanaito reviewed Dec 1, 2022

View reviewed changes

package.json Outdated Show resolved Hide resolved

kettanaito reviewed Dec 1, 2022

View reviewed changes

package.json Outdated Show resolved Hide resolved

kettanaito reviewed Dec 1, 2022

View reviewed changes

tsup.config.ts Outdated Show resolved Hide resolved

kettanaito reviewed Dec 6, 2022

View reviewed changes

cdimitroulas force-pushed the feat/bundle-the-library branch from 27459f4 to c1ab348 Compare December 6, 2022 12:42

feat: bundle the library using tsup

ffacd94

re mswjs#283

cdimitroulas force-pushed the feat/bundle-the-library branch from c1ab348 to ffacd94 Compare December 6, 2022 12:42

cdimitroulas marked this pull request as ready for review December 6, 2022 12:44

chore: add "RemoteHttpInterceptor" entrypoint

875aa32

Merge branch 'main' into HEAD

cf2711c

fix: reordered exports

83901ea

Merge branch 'main' into feat/bundle-the-library

dbe49d0

mattcosta7 mentioned this pull request Feb 2, 2023

ESM support + treeshaking unused bits mswjs/msw#1384

Closed

1 task

kettanaito added 2 commits February 6, 2023 16:26

fix: use TextEncoder shim vs "web-encoding"

a8a2216

Merge branch 'main' into feat/bundle-the-library

4fc8dbd

kettanaito mentioned this pull request Feb 13, 2023

feat!: adopt the global Fetch API mswjs/msw#1436

Merged

34 tasks

Merge branch 'main' into feat/bundle-the-library

e374aa6

kettanaito approved these changes Feb 18, 2023

View reviewed changes

chore: remove unused requestId parameter

26b53c1

kettanaito merged commit 29acc59 into mswjs:main Feb 18, 2023

cdimitroulas mentioned this pull request Feb 22, 2023

Interceptor presets are no longer accessible for import #345

Closed

kettanaito mentioned this pull request Feb 27, 2023

Distribute interceptors properly #52

Closed

feat: distribute the library for browser and node in ESM and CJS #313

feat: distribute the library for browser and node in ESM and CJS #313

Conversation

cdimitroulas commented Nov 30, 2022 • edited Loading

cdimitroulas commented Nov 30, 2022

mattcosta7 commented Nov 30, 2022

kettanaito commented Dec 1, 2022

mattcosta7 commented Dec 1, 2022

cdimitroulas commented Dec 2, 2022

cdimitroulas commented Dec 6, 2022

kettanaito Dec 6, 2022

Choose a reason for hiding this comment

cdimitroulas Dec 6, 2022

Choose a reason for hiding this comment

kettanaito Dec 6, 2022

Choose a reason for hiding this comment

kettanaito commented Dec 6, 2022

cdimitroulas commented Dec 6, 2022

kettanaito commented Dec 7, 2022

cdimitroulas commented Dec 7, 2022

cdimitroulas commented Dec 7, 2022

cdimitroulas commented Dec 7, 2022

mattcosta7 commented Dec 7, 2022 • edited Loading

cdimitroulas commented Dec 7, 2022 • edited Loading

cdimitroulas commented Dec 7, 2022

cdimitroulas commented Dec 7, 2022

cdimitroulas commented Jan 23, 2023

kettanaito commented Jan 23, 2023

cdimitroulas commented Jan 31, 2023

kettanaito commented Feb 1, 2023

kettanaito commented Feb 1, 2023

kettanaito commented Feb 1, 2023 • edited Loading

kettanaito commented Feb 1, 2023

Solution to web-encoding

kettanaito commented Feb 1, 2023

Issue: Jest doesn't understand the exports field

cdimitroulas commented Feb 1, 2023

kettanaito commented Feb 1, 2023

kettanaito commented Feb 1, 2023

mattcosta7 commented Feb 1, 2023 • edited Loading

kettanaito commented Feb 6, 2023

kettanaito commented Feb 6, 2023 • edited Loading

Trying this with the Fetch API branch

kettanaito left a comment

Choose a reason for hiding this comment

kettanaito commented Feb 18, 2023

Released: v0.21.0 🎉

blaenk commented Feb 20, 2023

cdimitroulas commented Feb 20, 2023

blaenk commented Feb 21, 2023

cdimitroulas commented Nov 30, 2022 •

edited

Loading

mattcosta7 commented Dec 7, 2022 •

edited

Loading

cdimitroulas commented Dec 7, 2022 •

edited

Loading

kettanaito commented Feb 1, 2023 •

edited

Loading

Solution to `web-encoding`

Issue: Jest doesn't understand the `exports` field

mattcosta7 commented Feb 1, 2023 •

edited

Loading

kettanaito commented Feb 6, 2023 •

edited

Loading