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

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

Closed
4 tasks done
funes79 opened this issue Nov 1, 2024 · 17 comments · Fixed by #2349
Closed
4 tasks done

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

funes79 opened this issue Nov 1, 2024 · 17 comments · Fixed by #2349
Assignees
Labels
bug Something isn't working scope:browser Related to MSW running in a browser

Comments

@funes79
Copy link

funes79 commented Nov 1, 2024

Prerequisites

Environment check

  • I'm using the latest msw version
  • I'm using Node.js version 18 or higher

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)

@funes79 funes79 added bug Something isn't working needs:triage Issues that have not been investigated yet. scope:browser Related to MSW running in a browser labels Nov 1, 2024
@robbtraister
Copy link
Contributor

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
Copy link
Member

@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 kettanaito changed the title Upgrade to 2.6.0 breaks interception Interception broken due to instanceof handlers check (dual publish) Nov 5, 2024
@kettanaito
Copy link
Member

@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
Copy link
Contributor

@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
Copy link
Member

@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
Copy link
Contributor

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
Copy link
Member

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
Copy link
Contributor

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
Copy link
Contributor

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
Copy link
Member

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
Copy link
Contributor

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
Copy link

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

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

@kettanaito
Copy link
Member

I've opened a fix at #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 kettanaito self-assigned this Nov 6, 2024
@kettanaito kettanaito removed the needs:triage Issues that have not been investigated yet. label Nov 6, 2024
@kettanaito
Copy link
Member

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
Copy link

rolule commented Nov 19, 2024

@kettanaito thank you for looking into this. It seems that playwright-msw is no longer maintained (last commit was a year ago). I opened valendres/playwright-msw#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
Copy link
Member

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
Copy link

rolule commented Nov 20, 2024

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

@github-actions github-actions bot locked and limited conversation to collaborators Dec 6, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
bug Something isn't working scope:browser Related to MSW running in a browser
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants