mswjs / interceptors

Low-level network interception library.
https://npm.im/@mswjs/interceptors
MIT License
527 stars 119 forks source link

Unintended side-effect when importing the module #504

Closed JAspeling closed 5 months ago

JAspeling commented 5 months ago

When using the @mswjs/interceptors, it fails when running tests because jsdom is not aware of TextEncoder ( issue #2524 ) when the tests are run, and you get the following error:

ReferenceError: TextEncoder is not defined

This comes from this module (bufferUtils), which creates an instance of the TextEncoder as a side-effect, even when you are not using the object.

A possible solution is to add the TextEncoder to the (jest) globals or test setup. Still, in our case, we have a mono-repo with many libraries and projects, and adding this config to all these projects does not feel like a good solution - and we ran into some issues with the global test setup.

A simple fix for this is removing the side effect and only creating (and reusing) the TextEncoder object when needed.

JAspeling commented 5 months ago

Fixes #505

kettanaito commented 5 months ago

A possible solution is to add the TextEncoder to the (jest) globals or test setup

This is the solution. TextEncoder and TextDecoder are global APIs in both browser and Node.js. Have been for years. It's Jest what's stripping your environment off these globals. And it's on the Jest's level that it's supposed to be fixed.

I know it's annoying but that the price we all pay for using a bit outdated tech. Luckily, polyfilling this is possible: https://mswjs.io/docs/migrations/1.x-to-2.x#requestresponsetextencoder-is-not-defined-jest

skondrashov commented 5 months ago

It seems reasonable for jest to strip these variables, and for jsdom to be responsible for setting up my environment so that they are defined. I know that still makes it a jsdom issue rather than an msw issue, but the folks over at jsdom don't seem to think it's their issue either. As long as the react-testing-library docs recommend using jest and also use msw in their examples, it doesn't feel like a great experience to find out I have to use this polyfill. Which I know would again make it a react-testing-library problem, not an msw problem, but as a user I was just hoping to follow a tutorial and have tests in my project at the end.

PS The polyfill doesn't work, giving ReferenceError: ReadableStream is not defined after undici is installed.

FrankMerema commented 5 months ago

I've experienced the same behaviour using the interceptor library and had to add the TextEncoder specifically to the the global jest setup. I agree that this is something probably related to JSDOM as well, but on the other side this code is now introducing a side effect impacting all users. Even when I don't use the TextEncoder I now have to add a polyfill for it.

Secondly I'm wondering why we can't merge this PR and fix it on this side? If I look into the history of the bufferUtils originally the TextEncoder was created every time on the fly: https://github.com/mswjs/interceptors/blob/e305d3815851dbdfd674e2b412f2c2e0288fc1de/src/utils/bufferUtils.ts. I don't really can find why this has changed in the past (as in why the TextEncoder instantiation is pulled outside to a global variable)

I hope we can revisit this decision and see if it can be fixed for the users of this awesome library!

kettanaito commented 5 months ago

PS The polyfill doesn't work, giving ReferenceError: ReadableStream is not defined after undici is installed.

You have to polyfill ReadableStream too! Alongside a bunch of other things Jest takes away from you. I'm sorry for this experience.

I agree that this is something probably related to JSDOM as well, but on the other side this code is now introducing a side effect impacting all users. Even when I don't use the TextEncoder I now have to add a polyfill for it.

The cost of using Jest. Consider migrating to Vitest where these problems don't exist.

There will be no changes to the libraries in this regard because they ship valid JavaScript code. The fact that third-party tooling cannot handle it is, unfortunately, on the tooling's side and must be addressed accordingly.

Secondly I'm wondering why we can't merge this PR and fix it on this side? If I look into the history of the bufferUtils originally the TextEncoder was created every time on the fly:

This is not a fix. It's a workaround to support a broken environment (Jest+JSDOM). I'm extremely reluctant when it comes to shipping workarounds.

as in why the TextEncoder instantiation is pulled outside to a global variable

Because this is how one uses TextEncoder/TextDecoder. They are global classes that can be constructed on the root scope and reused.

I hope we can revisit this decision and see if it can be fixed for the users of this awesome library!

Trust me, I'd like to see this fixed as well. But the fix belongs to Jest, and I hope they will find time to get JSDOM working correctly. Meanwhile, you have two options:

  1. Configure Jest to polyfill back the global APIs.
  2. Migrate to Vitest and forget about this, and many other Jest-related problems.

I highly recommend the latter.

FrankMerema commented 5 months ago

Thanks for the quick reply @kettanaito!

I agree with almost everything you're mentioning, but I do have some questions left:

This is not a fix. It's a workaround to support a broken environment (Jest+JSDOM). I'm extremely reluctant when it comes to shipping workarounds.

I don't feel this answers the question you've quoted, as in why has the behavior changed between the versions and what did it fix?

Because this is how one uses TextEncoder/TextDecoder. They are global classes that can be constructed on the root scope and reused.

For this, since this file is being exported via a generic barrel file it's "always" included when running static code compilation as Jest uses. So even when the user of the library is not using the file it still ends up in their (test) code. This is of course not a bad thing, but IMHO code then shouldn't use side effects as creating variables on the global scope.

skondrashov commented 5 months ago

@kettanaito I understand where you're coming from as a developer, but as a user, I just need something that works. Jest with something to mock functions that supports jest is probably going to be that thing for many people, I don't want to switch to a test runner I've never heard of because this library I'd also never heard of until it showed up in the RTL docs (which recommend jest) doesn't support jest. Of course it's your library but I'm just saying I feel like it would help you in terms of people using your library if it wasn't so painful to get it to work - you are technically right that it's not your fault (though i think, and it appears that the folks at jsdom agree, that it is jsdom's fault, not jest's fault - vitest might be leaking node variables into the browser environment but that's not necessarily correct. that jsdom doesn't provide a full browser environment is of course suboptimal but jest is right to clear out the node vars in my opinion), but again from a user's perspective I just want to test my code. I do think msw is a very cool project but I don't really understand what it's doing in the RTL docs, and I think many people would prefer to use something else unless RTL officially recommends switching away from jest or msw becomes compatible with jest and jsdom.