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.

Sign up for GitHub

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

Jump to bottom

Fix TextEncoder.encode not referencing same global Uint8Array constructor #9261

Merged
merged 7 commits into from
Dec 7, 2019
Merged

Fix TextEncoder.encode not referencing same global Uint8Array constructor #9261

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
ab8501c
Update reference of global.Uint8Array
wsmd Dec 4, 2019
cff81ee
Update test
wsmd Dec 4, 2019
c4e342f
Remove error snapshot assertion due to conditional testing
wsmd Dec 4, 2019
bfffce0
Update reference of global.Uint8Array
wsmd Dec 6, 2019
9c17fde
Add changelog entry
wsmd Dec 6, 2019
d5200ae
add failing test for Uint8Array
wsmd Dec 6, 2019
8858ed4
Merge branch 'master' into fix-equality-for-iint8array-textencoder
SimenB Dec 7, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 15 additions & 0 deletions packages/expect/src/__tests__/matchers.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@ afterAll(() => {

/* global BigInt */
const isBigIntDefined = typeof BigInt === 'function';
const isTextEncoderDefined = typeof TextEncoder === 'function';

it('should throw if passed two arguments', () => {
expect(() => jestExpect('foo', 'bar')).toThrow(
Expand Down Expand Up @@ -734,6 +735,20 @@ describe('.toEqual()', () => {
});
});

if (isTextEncoderDefined) {
[
[new Uint8Array([97, 98, 99]), new Uint8Array([97, 98, 99])],
[new TextEncoder().encode('abc'), new Uint8Array([97, 98, 99])],
].forEach(([a, b]) => {
test(`{pass: true} expect(${stringify(a)}).not.toEqual(${stringify(
b,
)})`, () => {
jestExpect(a).toEqual(b);
expect(() => jestExpect(a).not.toEqual(b)).toThrowError('toEqual');
});
});
}

if (isBigIntDefined) {
[
[BigInt(1), BigInt(1)],
Expand Down
1 change: 1 addition & 0 deletions packages/jest-environment-node/src/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -58,6 +58,7 @@ class NodeEnvironment implements JestEnvironment {
) {
global.TextEncoder = TextEncoder;
global.TextDecoder = TextDecoder;
global.Uint8Array = Uint8Array;
Copy link
Collaborator

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

Copy link
Contributor Author

@wsmd wsmd Dec 5, 2019 •
edited
Loading

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 TextEncoder isn't defined then re-assigning Uint8Array isn't really needed (#9261 (comment)).

// 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:

https://github.com/facebook/jest/blob/c4e342f86cac12a965ce3974212102a88d0fef55/packages/jest-environment-node/src/index.ts#L47-L49

Copy link
Collaborator

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?

Copy link
Contributor Author

@wsmd wsmd Dec 5, 2019 •
edited
Loading

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 👍

}
// queueMicrotask is global in Node >= 11
if (typeof queueMicrotask !== 'undefined') {
Expand Down