fix: use conditional exports #134

kettanaito · 2024-01-18T12:23:01Z

📦 Published PR as canary version: 1.10.1--canary.134.37dfc82.0

✨ Test out this PR locally via:

npm install msw-storybook-addon@1.10.1--canary.134.37dfc82.0
# or 
yarn add msw-storybook-addon@1.10.1--canary.134.37dfc82.0

The fundamental issue is that we have a single module that does three requires to different environments. We think we make those requires conditional by introducing if checks but those are runtime conditions, not build-time.

The simplest route to take is to split the initialize module into three, environment0-dependent modules, and resolve them conditionally on build-time, since we know during the build the target we're building to (browser/node/react native).

vercel · 2024-01-18T12:23:07Z

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name	Status	Preview	Comments	Updated (UTC)
msw-storybook-addon	✅ Ready (Inspect)	Visit Preview	💬 Add feedback	Jan 29, 2024 11:11am

kettanaito · 2024-01-18T12:41:20Z

A single JSDOM test is still failing because it uses the browser module resolution despite Jest config telling it not to:

FAIL  src/demos/fetch/AddonOnNode.test.jsx
  ● Test suite failed to run

    Invariant Violation: [MSW] Failed to execute `setupWorker` in a non-browser environment. Consider using `setupServer` for Node.js environment instead.

      31 | var api;
      32 | function initialize(options, initialHandlers = []) {
    > 33 |   const worker = (0, import_browser.setupWorker)(...initialHandlers);
         |                                                ^
      34 |   worker.start(options);
      35 |   api = worker;
      36 |   return worker;

      at setupWorker (../../node_modules/msw/lib/browser/index.js:754:10)
      at initialize (../msw-addon/dist/index.browser.js:[33](https://github.com/mswjs/msw-storybook-addon/actions/runs/7570077990/job/20614712950?pr=134#step:5:34):48)
      at Object.<anonymous> (src/demos/fetch/AddonOnNode.test.jsx:12:11)

I may be missing some context around this test to understand why this happens. I'm quite confuse as to what resolves msw-storybook-addon in that test at all, there's no alias in Jest or on the monorepo level. Tool magic.

kettanaito · 2024-01-18T12:57:51Z

packages/msw-addon/tsup.config.ts

+  target: 'node18',
+  format: 'cjs',
+  esbuildOptions(options) {
+    options.alias = {


We can technically keep a single initialize.ts and then use build-time condition via define to strip away any conditional imports. But I find it risky because we are opening the doors to accidentally mixing environments if we forget to check for them.

Our functionality here is tiny. There's no harm in repeating.

kettanaito · 2024-01-18T12:58:12Z

packages/msw-addon/src/loader.ts

+    api.resetHandlers()
+
+    if (msw) {
+      if (Array.isArray(msw) && msw.length > 0) {


What I'd much like to see reuse is this logic. It's identical between the decorator and the loader and moving it away looks like a good direction to take.

My understanding is we can just get rid of the decorator entirely. That it was the original implementation but has timing issues that are solved by the loader.

@yannbf, can you please confirm that?

If we can drop mswDecorator, we can simplify our addon tremendously. Now is also the right time to drop it, since the MSW 2.0 introduction is a breaking change anyway.

I'd also like to discuss the general DX with you. I feel that we can move the initialize call to the MSW loader and have a better end developer-experience for everyone. My assumption is that we are asking the users to call initialize in Storybook's config to kick off MSW earlier. Is that the case?

Yup I had plans to remove the mswdecorator, but it's not super simple.

The problem is that it makes it much more difficult for people who reuse stories in tests (especially because portable stories do not support loaders), and this might be a big change for them, and thus requires a proper migration guide.

Regarding initialize, we could provide the code internally but then we remove the customizing aspect of it, which is needed in many cases. We could also make it optional, and call the internal code only if initialize is not called by the user. The side effect to this, again, is that it will break the story reuse scenario (because portable stories won't access such presets)

Got it. Thanks for providing the much needed context here!

Regarding initialize, we could provide the code internally but then we remove the customizing aspect of it, which is needed in many cases.

Can't we provide the customization on the loader level though?

export function mswLoader(options) { return theActualLoader } { loaders: [mswLoader({ onUnhandledRequest: 'warn' }) }

shilman · 2024-01-18T15:36:15Z

packages/msw-addon/src/index.new.ts

@@ -0,0 +1,3 @@
+export { initialize, InitializeOptions } from '@build-time/initialize'


🤯 That's really cool...

Note: it's just a fancy name to make it clear where this import will be resolved to. This import identifier is an alias in tsup.confug.ts that resolves it to different modules (initialize.browser.ts, initialize.node.ts, etc) on build time.

kettanaito · 2024-01-19T17:19:23Z

Added the right TypeScript configuration to support the right types depending on where you import msw-storybook-addon. For example, the ./dist/index.browser.d.ts will contain msw/browser types:

import { RequestHandler } from 'msw';
import { SetupWorker } from 'msw/browser';

type InitializeOptions = Parameters<SetupWorker['start']>[0];
declare function initialize(options?: InitializeOptions, initialHandlers?: RequestHandler[]): SetupWorker;

While ./dist/index.node.d.ts will refer to msw/node:

import { RequestHandler } from 'msw';
import { SetupServer } from 'msw/node';

type InitializeOptions = Parameters<SetupServer['listen']>[0];
declare function initialize(options?: InitializeOptions, initialHandlers?: RequestHandler[]): SetupServer;

kettanaito · 2024-01-19T17:23:01Z

packages/docs/jest.config.js

@@ -19,6 +19,9 @@ module.exports = {
  setupFiles: ['./src/fetch-polyfill.js'],
  setupFilesAfterEnv: ['./src/setupTests.js'],
  testEnvironment: 'jsdom',
+  testEnvironmentOptions: {
+    customExportConditions: [''],


We must use node-like module resolution in JSDOM. JSDOM uses a browser module resolution by default and that's a rather big oversight on their part.

kettanaito · 2024-01-19T17:23:29Z

packages/msw-addon/package.json

+  "types": "./dist/index.browser.d.ts",
+  "exports": {
+    "browser": {
+      "types": "./dist/index.browser.d.ts",


Also providing the right types since depending on the environment, you will be importing either msw/browser or msw/node when using initialize.

kettanaito · 2024-01-19T17:24:11Z

packages/msw-addon/src/initialize.d.ts

+  | Parameters<SetupServer['listen']>[0]
+  | Parameters<SetupWorker['start']>[0]
+
+export declare function initialize(


This is really an ambiguous placeholder module used for local development, where we don't yet know which environment the code will compile to. This must never be shipped. This is why explicit environment TypeScript configs never even include this file.

kettanaito · 2024-01-19T17:24:50Z

packages/msw-addon/src/loader.ts

+import { applyRequestHandlers } from './applyRequestHandlers'
+
+export const mswLoader = async (context: Context) => {
+  applyRequestHandlers(context.parameters.msw)


@yannbf, I think you will love how simple these functions become! I've also moved the singleton api access to applyRequestHandlers, which means we keep a single point of entry when we decide to refactor something.

kettanaito · 2024-01-19T17:25:57Z

packages/msw-addon/tsconfig.browser.json

+  "compilerOptions": {
+    "baseUrl": ".",
+    "paths": {
+      "@build-time/initialize": ["./src/initialize.browser.ts"]


The idea of environment-specific TypeScript configs is really simple:

Each build in tsup.config.ts points to a different tsconfig.<env>.json.

Each different tsconfig.<env>.json resolves the @build-time/initialize import differently on type-level (that's why we provide a standalone import alias in esbuildOptions for build-level module resolution too.

This way each compiled .d.ts file comes with the right types and msw/* imports.

kettanaito · 2024-01-19T17:26:32Z

@mattcosta7, hi, Matt 👋 May I ask your opinion on the direction we are taking with this pull request? If you have a moment, of course.

mattcosta7 · 2024-01-22T14:57:38Z

@mattcosta7, hi, Matt 👋 May I ask your opinion on the direction we are taking with this pull request? If you have a moment, of course.

A quick glance and this makes sense, but I don't know too much about the build/runtimes of storybook addons offhand to be sure this makes sense. Conceptually breaking this in the way described looks reasonable though

nit: 1 test seems to be failing due to environment

FAIL  src/demos/fetch/AddonOnNode.test.jsx
  ● Test suite failed to run

    Invariant Violation: [MSW] Failed to execute `setupWorker` in a non-browser environment. Consider using `setupServer` for Node.js environment instead.

      31 | var api;
      32 | function initialize(options, initialHandlers = []) {
    > 33 |   const worker = (0, import_browser.setupWorker)(...initialHandlers);
         |                                                ^
      34 |   worker.start(options);
      35 |   api = worker;
      36 |   return worker;

      at setupWorker (../../node_modules/msw/lib/browser/index.js:754:10)
      at initialize (../msw-addon/dist/index.browser.js:[33](https://github.com/mswjs/msw-storybook-addon/actions/runs/7587019471/job/20666449469?pr=134#step:5:34):48)
      at Object.<anonymous> (src/demos/fetch/AddonOnNode.test.jsx:12:11)

It's not super clear what/if any configuration will be needed for end users, compared to the current version which is minimal?

kettanaito · 2024-01-22T16:29:02Z

@mattcosta7, I think the test is failing because it runs Storybook and somehow the test (jest?)(thinks it should use the browser resolution strategy). I assume this is what's happening:

Test -> Story -> import msw-storybook-addon - (browser resolution) -> msw/browser -> throws because we are still in Node.js.

Strangely, Jest config to opt-out from this in JSDOM doesn't seem to have any effect. Perhaps @yannbf knows a bit more.

yannbf · 2024-01-22T16:34:32Z

@mattcosta7, I think the test is failing because it runs Storybook and somehow the test (jest?)(thinks it should use the browser resolution strategy). I assume this is what's happening:

Test -> Story -> import msw-storybook-addon - (browser resolution) -> msw/browser -> throws because we are still in Node.js.

Strangely, Jest config to opt-out from this in JSDOM doesn't seem to have any effect. Perhaps @yannbf knows a bit more.

Hey there! 👋

I am not sure what you mean by running Storybook, but the tests which we run in Node, just use an API from Storybook (nothing more than a javascript function), it doesn't do anything with Storybook's binary or the browser.

I think the path forward in this repo is to migrate the example code to use Vitest (which is in this PR), but it's important to know whether this change would work with Jest as well

I'll take a look at this as soon as I get some time. I just got back from holidays and there are tons of stuff to catch up to.

kettanaito · 2024-01-22T18:48:14Z

@yannbf, happy to have you back!

I think we should fix the existing test (it's the only one failing), and I also suspect that the issue lies in how that test is configured. We can migrate the entire repo to use Vitest later, I wouldn't add it on top of an already long-pending changes. Let's bring MSW 2.0 to the Storybook users as soon as we can.

This 99% has to do with export conditions. I'm just confused why the general Jest config that fixes this doesn't do anything for that particular test.

yannbf · 2024-01-29T11:37:08Z

Awesome stuff here @kettanaito ! thank you so much for your help <3 I fixed the last bits and brought the getWorker function back that was removed (probably by mistake). I'll continue the work on the other PR!

kettanaito · 2024-01-29T15:39:10Z

packages/docs/jest.config.js

-  moduleNameMapper: { '^.+\\.module\\.(css|sass|scss)$': 'identity-obj-proxy' },
+  moduleNameMapper: {
+    '^.+\\.module\\.(css|sass|scss)$': 'identity-obj-proxy',
+    '^msw-storybook-addon$': '<rootDir>/../msw-addon/dist/index.node.js',


@yannbf this makes sense just make sure this isn't needed for the end developer who will be using Storybook and the add-on for integration testing.

* Breaking change: Support MSW 2.0 Co-authored-by: Gert Hengeveld <info@ghengeveld.nl> * update example to use MSW 2.0 handlers * temporarily disable tests in release workflow * update jest config to pass tests * fix path * worker * alias msw/native * update peer dependency range * upgrade example storybook * fix: use conditional exports (#134) * fix: use conditional exports * chore: keep "dist" as build directory * chore: remove custom webpackFinal config * chore: prevent browser-style resolution in jsdom tests * chore: remove old code * chore: use "index.ts" * fix: use environment-based tsconfig.json * chore: remove unused imports * fix: abstract "api.use()" to "applyRequestHandlers" * read getWorker export * add types for getWorker --------- Co-authored-by: Yann Braga <yannbf@gmail.com> * bring back tests * Filter requests that are not relevant to the user * make sure to reset the handlers * feat!(msw-addon): fix package exports and generate esm build (#138) this updates conditional exports so that both types and dependencies can be resolved. this also migrates build to generating esm by default for a more standards based packaging approach with strong support in modern tooling like webpack and vite. resolves #137 * fix(loader.ts): fix possible undefined (#141) If msw is running in fallback mode, navigator.serviceWorker may not be defined. * update auto * update to yarn 4 * upgrade chromatic * upgrade CI actions * fix return types * add yarn binary * fix jest script * fix chromatic script * fix chromatic script * update package.json files * refactor tsup config * remove unnecessary dependency * temp: downgrade to yarn1 * export applyRequestHandlers * update docs [skip ci] * update tests * upgrade to Storybook 8 * improve documentation on portable stories * deprecate mswDecorator * deprecate msw parameter as an array * migrate docs example to vite (#128) * migrate docs example to vite * migrate from jest to vitest * add rollup workaround * migrate example to Typescript --------- Co-authored-by: Gert Hengeveld <info@ghengeveld.nl> Co-authored-by: Matthew Costabile <mattcosta7@github.com> Co-authored-by: Artem Zakharchenko <kettanaito@gmail.com> Co-authored-by: Christian Murphy <christian.murphy.42@gmail.com> Co-authored-by: Alex Muñoz <AlexMunoz@users.noreply.github.com>

fix: use conditional exports

676bec1

kettanaito changed the base branch from main to feat/support-msw-2.0 January 18, 2024 12:23

vercel bot had a problem deploying to Preview January 18, 2024 12:23 Failure

chore: keep "dist" as build directory

58a7c6c

vercel bot had a problem deploying to Preview January 18, 2024 12:28 Failure

chore: remove custom webpackFinal config

6ef71b8

vercel bot deployed to Preview January 18, 2024 12:31 View deployment

chore: prevent browser-style resolution in jsdom tests

851c8da

kettanaito added the skip-release Preserve the current version when merged label Jan 18, 2024

vercel bot deployed to Preview January 18, 2024 12:39 View deployment

kettanaito mentioned this pull request Jan 18, 2024

BREAKING CHANGE: Support MSW 2.0 #122

Merged

kettanaito commented Jan 18, 2024

View reviewed changes

shilman reviewed Jan 18, 2024

View reviewed changes

kettanaito added 4 commits January 19, 2024 17:55

chore: remove old code

31d6fbf

chore: use "index.ts"

a6dadb2

fix: use environment-based tsconfig.json

6ba2e6c

chore: remove unused imports

14b0ba7

vercel bot deployed to Preview January 19, 2024 17:11 View deployment

fix: abstract "api.use()" to "applyRequestHandlers"

9892d86

vercel bot deployed to Preview January 19, 2024 17:18 View deployment

kettanaito commented Jan 19, 2024

View reviewed changes

yannbf added patch Increment the patch version when merged and removed skip-release Preserve the current version when merged labels Jan 29, 2024

read getWorker export

f48a200

vercel bot deployed to Preview January 29, 2024 11:00 View deployment

add types for getWorker

966f069

vercel bot deployed to Preview January 29, 2024 11:11 View deployment

yannbf marked this pull request as ready for review January 29, 2024 11:37

yannbf merged commit 37dfc82 into feat/support-msw-2.0 Jan 29, 2024
6 of 8 checks passed

yannbf deleted the fix/exports branch January 29, 2024 11:37

kettanaito commented Jan 29, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: use conditional exports #134

fix: use conditional exports #134

kettanaito commented Jan 18, 2024 •

edited by github-actions bot

vercel bot commented Jan 18, 2024 •

edited

kettanaito commented Jan 18, 2024

kettanaito Jan 18, 2024

kettanaito Jan 18, 2024

shilman Jan 18, 2024

kettanaito Jan 18, 2024

yannbf Jan 18, 2024 •

edited

kettanaito Jan 18, 2024

shilman Jan 18, 2024 •

edited

kettanaito Jan 18, 2024

kettanaito commented Jan 19, 2024

kettanaito Jan 19, 2024

kettanaito Jan 19, 2024

kettanaito Jan 19, 2024

kettanaito Jan 19, 2024

kettanaito Jan 19, 2024

kettanaito commented Jan 19, 2024

mattcosta7 commented Jan 22, 2024

kettanaito commented Jan 22, 2024

yannbf commented Jan 22, 2024

kettanaito commented Jan 22, 2024

yannbf commented Jan 29, 2024

kettanaito Jan 29, 2024

		@@ -0,0 +1,3 @@
		export { initialize, InitializeOptions } from '@build-time/initialize'

fix: use conditional exports #134

fix: use conditional exports #134

Conversation

kettanaito commented Jan 18, 2024 • edited by github-actions bot

vercel bot commented Jan 18, 2024 • edited

kettanaito commented Jan 18, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yannbf Jan 18, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shilman Jan 18, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kettanaito commented Jan 19, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kettanaito commented Jan 19, 2024

mattcosta7 commented Jan 22, 2024

kettanaito commented Jan 22, 2024

yannbf commented Jan 22, 2024

kettanaito commented Jan 22, 2024

yannbf commented Jan 29, 2024

Choose a reason for hiding this comment

kettanaito commented Jan 18, 2024 •

edited by github-actions bot

vercel bot commented Jan 18, 2024 •

edited

yannbf Jan 18, 2024 •

edited

shilman Jan 18, 2024 •

edited