-
-
Notifications
You must be signed in to change notification settings - Fork 125
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
Unintended side-effect when importing the module #504
Conversation
Fixes #505 |
This is the solution. 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 |
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 |
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 hope we can revisit this decision and see if it can be fixed for the users of this awesome library! |
You have to polyfill
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.
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.
Because this is how one uses
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:
I highly recommend the latter. |
Thanks for the quick reply @kettanaito! I agree with almost everything you're mentioning, but I do have some questions left:
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?
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. |
@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. |
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:
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.