Closed JAspeling closed 9 months ago
Hi, @JAspeling. Thanks for reporting this!
This isn't a bug with the Interceptors. I'm talking a bit more about why here: https://github.com/mswjs/interceptors/pull/504#issuecomment-1934869233
Hi @kettanaito , I know that the bug originates from the fact that jsdom doesn't add these as globals - but the issue I'm trying to resolve is not necessarily because of that.
In our project, we import from @mswjs/interceptors
- the barrel file in src/index.ts exports the following:
export { encodeBuffer, decodeBuffer } from './utils/bufferUtils'
This means that the following code gets executed when importing anything from @mswjs/interceptors
:
const encoder = new TextEncoder()
So basically, even if I do not depend on TextEncoder
in any of our code, this side effect (creating a new instance of TextEncoder
) in the bufferUtils
module causes an error because TextEncoder
is indeed not available as a global when the jest tests execute.
Removing this side-effect from the module fixes the issue for us without the need for any polyfills or declaring it in the test setup - is there a reason why the suggested change #504 is not beneficial to this project?
Replied here but for others to find this issue: #504 (comment)
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!
@JAspeling, this is derived, I understand it. But deferring TextEncoder
access won't solve anything. There may be other areas in this library, or other libraries, that rely on the global TextEncoder
API, and it will still fail. Deferring all those accesses is not fixing anything. The bottom line in, new TextEncoder()
in a root scope is a valid usage of JavaScript. It's Jest who makes it invalid.
The root cause is that a valid global class in JavaScript is taken away from you by Jest. Since Jest introduces that mutation, it's on Jest's level that the issue must be addressed. I'm not going to add a workaround to support an outdated piece of tech. This is how we drag the entire ecosystem down.
Jest is old. Jest doesn't support ESM, couldn't add proper support for years, and is unlikely to add one in the future. Jest meddles with your environment in a very, very bad way. I've used Jest for years and loved it. As we enter a modern era of JavaScript, tools like Jest have to either adapt or have to go. My developer life has gotten tremendously better after I migrated my projects to Vitest, which I highly recommend you do. The fact that an old tool demands so much of your mental energy is unacceptable.
I hope this explains well why I'm not in favor of this workaround.
When using the @mswjs/interceptors, it fails when running tests because jsdom is not aware of TextEncoder ( https://github.com/jsdom/jsdom/issues/2524 ) when the tests are run, and you get the following error:
The error occurs even when you don't use the TextEncoder - and originates from a module (bufferUtils), which creates an instance of the TextEncoder as a side-effect even when you are not using the object.
504 attempts to fix this behaviour