mswjs / msw

Industry standard API mocking for JavaScript.
https://mswjs.io
MIT License
15.97k stars 519 forks source link

Interception broken due to `instanceof` handlers check (dual publish) #2346

Closed funes79 closed 2 weeks ago

funes79 commented 2 weeks ago

Prerequisites

Environment check

Browsers

Chromium (Chrome, Brave, etc.)

Reproduction repository

https://github.com/funes79/nocode

Reproduction steps

I tested 2.4.0, 2.3.0, 2.5.0, all works as expected. But upgrade to 2.6.0 breaks intercepting, in client components:

[MSW] Warning: intercepted a request without a matching request handler:

  • GET https://github.com/reviews2

If you still wish to intercept this unhandled request, please create a request handler for it.
Read more: https://mswjs.io/docs/getting-started/mocks

handlers file:

import { http, HttpResponse } from 'msw';

export const handlers = [
  http.get('*.js', () => passthrough()),
  http.get('github.com/reviews2', () => {
    console.log('MSW reviews2');
    // return HttpResponse.text('Hello, World!');
    return HttpResponse.json({ reviews: 'Hello, World2!' });
  }),
];

Current behavior

[MSW] Warning: intercepted a request without a matching request handler:

GET https://github.com/reviews2

Expected behavior

Should return: {"reviews":"Hello, World!"} [MSW] 14:41:28 GET https://github.com/reviews2 (200 OK)

robbtraister commented 2 weeks ago

I think this issue is caused by the new instanceof checks. Removing this block fixed the problem for me.

My initial assessment is that there is some inconsistency regarding when the .js file or the .mjs file is used. For my project (using playwright), it seems that my test code imports the .mjs file, but the test runner imports the .js file. Loading separate files results in the creation of separate classes (even if their functionality is identical), meaning the instanceof check fails.

kettanaito commented 2 weeks ago

@robbtraister, so the class identity doesn't survive due to different classes being loaded in different contexts. That's interesting. I'd like to know how come you get CJS and ESM builds running at the same time. What you are experiencing is one of the symptoms of dual-published hazard, and that's the root cause of the issue, not the instanceof check.

Could you please look around and see what causes different bundles to be loaded in your test case? Can it be that your Playwright config gets the CJS version of MSW, somehow?

kettanaito commented 2 weeks ago

@funes79, hi 👋 Did you by chance forget to push the code to your reproduction repo? It's empty. Please push the reproduction there and then I can take a look at this. None of our existing tests recreate this behavior so I suspect it has to do with the project setup.

robbtraister commented 2 weeks ago

@kettanaito it looks like the import inconsistency may be happening via the playwright-msw package. That package looks to be published only as CJS, but my app test code is using ESM.

I attempted to wire up my app test code without the playwright-msw package, but I can't seem to get it working. Are you aware of any example repos that integrate playwright without using playwright-msw?

kettanaito commented 2 weeks ago

@robbtraister, yes, we are using MSW and Playwright without any other packages at https://github.com/mswjs/examples/tree/main/examples/with-playwright. Could you give that repo a try and if there's still an issue submit reproduction steps?

robbtraister commented 2 weeks ago

That approach requires that msw be built into your application so that you're either shipping extra code in production or testing a non-production build.

It also makes writing tests much more complicated because the mock handlers must essentially be defined within the app, meaning it doesn't really support handler customization per test.

In order to customize the handlers, you have to wait until the page has already loaded (giving access to the msw library baked into the app), but at that point unintercepted requests may already have fired.

kettanaito commented 2 weeks ago

That approach requires that msw be built into your application so that you're either shipping extra code in production or testing a non-production build.

The way you integrate MSW for development already does this (ref). If anything, it's a single setup for any in-browser need, which I quite like.

It also makes writing tests much more complicated because the mock handlers must essentially be defined within the app, meaning it doesn't really support handler customization per test.

It does! I know the examples don't showcase that, but if you expose worker and the request handler namespace, you can provide per-test overrides.

await page.evaluate(() => {
  const { worker, http } = window.msw
  worker.use(http.get('/override', yep)
})

In order to customize the handlers, you have to wait until the page has already loaded (giving access to the msw library baked into the app), but at that point unintercepted requests may already have fired.

This is a valid point. How would you address it though? To register the worker, you have to visit the app, no matter if you do that in your app code or in a third-party package.

robbtraister commented 2 weeks ago

How would you address it though?

I don't have a good answer to this. It seems that the playwright-msw package registers msw RequestHandlers using the Page#route API from playwright, avoiding using the msw worker entirely.

This has worked really well up to this point, allowing us to register intercepts prior to test execution, and also using shared handlers that work in any msw context (we also use msw with vitest and storybook).

robbtraister commented 2 weeks ago

As a quick-fix to verify this is the real issue, I patched playwright-msw to re-export http and HttpResponse from msw, then updated my tests to import them from playwright-msw instead of msw. This ensures that I'm using the CJS version everywhere and all of my tests passed.

kettanaito commented 2 weeks ago

I don't have a good answer to this. It seems that the playwright-msw package registers msw RequestHandlers using the Page#route API from playwright, avoiding using the msw worker entirely.

Well, then it's not MSW at this point, but Playwright relying on the request handler API and using different interception algorithm (page.route()). Nothing against that! I was thinking of some serialization protocol for handlers between the test and browser threads since we do that all the time (worker <-> client, client <-> server over WebSocket). I may explore a closer Playwright integration in the future, that's been on my mind for a while now.

This ensures that I'm using the CJS version everywhere and all of my tests passed.

Which means the issue is in playwright-msw not distributing an ESM bundle. I suggest you raise it in that repository for the maintainers to track.

robbtraister commented 2 weeks ago

Which means the issue is in playwright-msw not distributing an ESM bundle. I suggest you raise it in that repository for the maintainers to track.

I understand this perspective as a maintainer, but as a consumer I find it unsatisfying. Essentially, you're suggesting that the entire ecosystem is effectively bifurcated, regardless of the work done to make CJS/ESM cross-compatible.

Would you consider an approach similar to Array.isArray()/Buffer.isBuffer()? we could add isRequestHandler/isHttpHandler/isWebSocketHandler props to each of the respective class constructors and use them in the conditional rather than the instanceof check?

patricklafrance commented 2 weeks ago

I have a similar issue but for a micro-frontends setup.

This block and this block are causing the issue for my setup.

kettanaito commented 2 weeks ago

I've opened a fix at https://github.com/mswjs/msw/pull/2349 that moves away from instanceof to prevent this issue from happening. We can use a private property to check the handler's type.

kettanaito commented 2 weeks ago

Released: v2.6.1 🎉

This has been released in v2.6.1!

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.

rolule commented 2 days ago

@kettanaito thank you for looking into this. It seems that playwright-msw is no longer maintained (last commit was a year ago). I opened https://github.com/valendres/playwright-msw/issues/104, which is similar to this issue, two weeks ago but unfortunately there has not been any answer from the maintainer so far.

I personally consider msw support for Playwright essential for a great integration testing experience. Since you wrote this above:

I may explore a closer Playwright integration in the future, that's been on my mind for a while now.

I wanted to ask you if you would consider migrating playwright-msw to the mswjs organization? I could create the first draft including ESM support. Since it is MIT licensed, I don't see a problem with that. I think this would really benefit users of Playwright+msw and would certainly be a plus for msw in general.

kettanaito commented 2 days ago

I am generally in favor of migrating community packages with large demand. Practically speaking, I need to look into playwright-msw and see if I agree with the approach taken there. If we have to rewrite it anyway, I would not spend time arranging an org migration, if that makes sense.

I have tool-specific integrations planned as an endeavor for the next year. For now, you can use patch-package to promote upgrades in packages that don't support them in conventional way. I will count this as +1 for @mswjs/playwright or similar. Thanks, @rolule.

Could you consider joining my Discord to stay in touch? We can also share thoughts about the Playwright integration there.

rolule commented 1 day ago

That makes sense, thank you for your explanation! I will join the Discord soon.