-
-
Notifications
You must be signed in to change notification settings - Fork 6.5k
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
Fix TextEncoder.encode not referencing same global Uint8Array constructor #9261
Fix TextEncoder.encode not referencing same global Uint8Array constructor #9261
Conversation
@@ -58,6 +58,7 @@ class NodeEnvironment implements JestEnvironment { | |||
) { | |||
global.TextEncoder = TextEncoder; | |||
global.TextDecoder = TextDecoder; | |||
global.Uint8Array = Uint8Array; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This needs its own if block to check whether typeof Uint8Array !== 'undefined'
is true. Feel free to also leave a comment on which Node version supports it, like it's done for other globals
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@thymikee that's not a bad idea!
My initial thinking was that this is only a workaround specific to the TextDecoder
as it seems to be referencing a different Uint8Array
constructor, so I grouped them all together in the same block. If (#9261 (comment)).TextEncoder
isn't defined then re-assigning Uint8Array
isn't really needed
// in Jest (using the node environment)
new TextEncoder().encode('').constructor === Uint8Array // false
// outside Jest (regular node runtime)
new TextEncoder().encode('').constructor === Uint8Array // true
Could you please help me understand under what conditions Uint8Array
would be undefined
? I think this helps me better understand the Jest architecture =)
Uint8Array
is very well supported since node v0.10
, unlike TextEncoder
that was only made available in the global namespace with node v11.0.0
(which is relatively recent). I'm not sure if there will be a case where Uint8Array
isn't defined (v0.10
dates back to 2013).
Isn't this similar? We're not checking for ArrayBuffers
here:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should be good with adding Uint8Array
next to the ArrayBuffer
then, wdyt @SimenB?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey hey! this conversation made me realize that this issue does not only occur with the global TextEncoder
(in Node v11), but also with require('util').TextEncoder
(in any version of node)! 😬
In Node v10 (or older), this test would still fail for the same reason:
// node v10 (importing TextEncoder from the 'util' module)
const {TextEncoder} = require('util');
test('TextEncoder.encode references the same global Uint8Array constructor', () => {
expect(new TextEncoder().encode('')).toBeInstanceOf(Uint8Array)
});
FAIL ./TextEncoder.test.js
✕ TextEncoder.encode references the same global Uint8Array constructor (5ms)
● TextEncoder.encode references the same global Uint8Array constructor
expect(received).toBeInstanceOf(expected)
Expected constructor: Uint8Array
Received constructor: Uint8Array
2 |
3 | test('TextEncoder.encode references the same global Uint8Array constructor', () => {
> 4 | expect(new TextEncoder().encode()).toBeInstanceOf(Uint8Array)
| ^
5 | });
at Object.toBeInstanceOf (TextEncoder.test.js:4:38)
We should be good with adding Uint8Array next to the ArrayBuffer then
Looks like after all we actually need to assign global.Uint8Array
regardless of TextEncoder
being global or not to support both cases 😅..
I'll go ahead and push the changes 👍
d532e21
to
bfffce0
Compare
This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs. |
Summary
Fixes #9204
This is a follow up for my comment: #9204 (comment)
Currently the following tests are failing in Jest:
It seems that the constructor of the
Uint8Array
returned fromTextEncoder.prototype.encode
and the globalUint8Array
constructor are not the same (causing some matchers to fail via theiterableEquality
); note that this only happens in the jest node environment.I'm not exactly sure why these two are referencing different constructors, or where either one of them is referencing a different one. I'm wondering if anyone has any pointers on this – I'm really curious!
That said, updating jest-environment-node's
global
to reference the actualUint8Array
seems to resolve this issue.Test plan
The following test should pass on this branch:
I added some new tests that would have failed outside of this PR.