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.

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

Merged
merged 7 commits into from Dec 7, 2019

Conversation

wsmd
Copy link
Contributor

@wsmd wsmd commented Dec 4, 2019

Summary

Fixes #9204

This is a follow up for my comment: #9204 (comment)

Currently the following tests are failing in Jest:

  • Using global TextEncoder (node v11 and above)
it('TextEncoder.encode references the same global Uint8Array constructor', () => {
  const actual = new TextEncoder().encode('foo');
  expect(actual).toEqual(new Uint8Array([102, 111, 111])); // fails
  expect(actual).toBeInstanceOf(Uint8Array); // fails
});
  • Importing TextEncoder from the 'util' module (not using global TextEncoder)
const {TextEncoder} = require('util');

it('TextEncoder.encode references the same global Uint8Array constructor', () => {
  const actual = new TextEncoder().encode('foo');
  expect(actual).toEqual(new Uint8Array([102, 111, 111])); // fails
  expect(actual).toBeInstanceOf(Uint8Array); // fails
});

It seems that the constructor of the Uint8Array returned from TextEncoder.prototype.encode and the global Uint8Array constructor are not the same (causing some matchers to fail via the iterableEquality); note that this only happens in the jest node environment.

// in Jest (using the node environment)
new TextEncoder().encode('').constructor === Uint8Array // false

// outside Jest (regular node runtime)
new TextEncoder().encode('').constructor === Uint8Array // true

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 actual Uint8Array seems to resolve this issue.

Test plan

The following test should pass on this branch:

test('TextEncoder references the same global Uint8Array constructor', () => {
  expect(new TextEncoder().encode('abc')).toBeInstanceOf(Uint8Array);
});

I added some new tests that would have failed outside of this PR.

@wsmd wsmd changed the title Fix equality of global Uint8Array and that of TextEncoder/TextDecoder Fix equality of global Uint8Array and that of TextEncoder Dec 4, 2019
@wsmd wsmd changed the title Fix equality of global Uint8Array and that of TextEncoder Fix equality of global Uint8Array and that of TextEncoder.encode Dec 4, 2019
@@ -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

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

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 👍

@wsmd wsmd force-pushed the fix-equality-for-iint8array-textencoder branch from d532e21 to bfffce0 Compare December 6, 2019 00:17
@wsmd wsmd changed the title Fix equality of global Uint8Array and that of TextEncoder.encode Fix TextEncoder.encode not referencing same global Uint8Array constructor Dec 6, 2019
@wsmd wsmd requested a review from thymikee December 6, 2019 00:29
@github-actions
Copy link

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.
Please note this issue tracker is not a help forum. We recommend using StackOverflow or our discord channel for questions.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators May 11, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

expect(...).toEqual(...) fails on strings encoded with TextEncoder
4 participants