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

0.34.3 doens't work with Uint8Array and TextEncoder #4043

Open
6 tasks done
wxiaoguang opened this issue Aug 29, 2023 · 23 comments
Open
6 tasks done

0.34.3 doens't work with Uint8Array and TextEncoder #4043

wxiaoguang opened this issue Aug 29, 2023 · 23 comments
Labels
p2-edge-case Bug, but has workaround or limited in scope (priority)

Comments

@wxiaoguang
Copy link

wxiaoguang commented Aug 29, 2023

Describe the bug

The test works in 0.34.2, but doesn't work in 0.34.3

Reproduction

test('toEqual', () => {
  expect([1]).toEqual([1]); // OK
  expect(new TextEncoder().encode('a')).toEqual(new TextEncoder().encode('a')); // OK
  expect(Uint8Array.from([97])).toEqual(new TextEncoder().encode('a')); // FAIL: AssertionError: expected Uint8Array[ 97 ] to deeply equal Uint8Array[ 97 ]
});

System Info

node: v20.5.0, macOS

Used Package Manager

npm

Validations

@paul-sachs
Copy link

I believe this also effects esbuild directly. Using vitest with a utility that calls esbuild results in:

Error: Invariant violation: "new TextEncoder().encode("") instanceof Uint8Array" is incorrectly false

This indicates that your JavaScript environment is broken. You cannot use
esbuild in this environment because esbuild relies on this invariant. This
is not a problem with esbuild. You need to fix your environment instead.

@wxiaoguang
Copy link
Author

wxiaoguang commented Aug 29, 2023

This change seems quite suspicious #3203

And this one: #3998 (b42cf)

@silverwind
Copy link
Contributor

I think #3998 is likely the culprit. Something about the Uint8Array returned by TextEncoder makes it fail, even thought those values are definitely structurally equal.

@silverwind
Copy link
Contributor

silverwind commented Aug 29, 2023

The cause is this removal, specifically:

TextEncoder (and TextDecoder) is a Node API and Uint8Array is a jsdom API. The Uint8Array that both produce are not comparable, even while structurally ident (I guess some prototype stuff differs which breaks toEqual).

jestjs/jest#9983 has more context and jsdom/jsdom#2524 would be the proper fix, but it's stale.

How does vitest want to proceed? I see two options:

  • Overwrite Uint8Array, TextEncoder, TextDecoder to the node implementations.
  • Overwrite nothing, let jsdom provide what it has and let the user polyfill themselves.

Both are fine with me, but the current situation where only TextEncoder and TextDecoder are from node is problematic.

@sheremet-va
Copy link
Member

sheremet-va commented Aug 30, 2023

The cause is this removal

The issue doesn't specify what mode it is running, so I assume it's not experimental-vm (TextEncoder should not even be available there). This line only affects --experimental-vm-threads.

But yeah, this one is quite hard to fix in general because jsdom uses its internal Uint8Array for its APIs, and Node.js uses its own implementation so they clash in different APIs, just like with URL.

@silverwind
Copy link
Contributor

silverwind commented Aug 30, 2023

Config is pretty much default configuration. TextEncoder is global in node since v11, so unless vitest deletes these globals (maybe it should), they will be present in non-node environments. I've worked around the issue now by converting to array for comparison.

deot added a commit to deot/dev that referenced this issue Sep 5, 2023
- `TextEncoder/TextDecoder` is a Node API
- `Uint8Array` is a jsdom API

vitest-dev/vitest#4043
deot added a commit to deot/dev that referenced this issue Sep 5, 2023
- `TextEncoder/TextDecoder` is a Node API
- `Uint8Array` is a jsdom API

vitest-dev/vitest#4043
deot added a commit to deot/dev that referenced this issue Sep 5, 2023
- `TextEncoder/TextDecoder` is a Node API
- `Uint8Array` is a jsdom API

vitest-dev/vitest#4043
@vighnesh153
Copy link

I've worked around the issue now by converting to array for comparison.

I am still seeing this issue. Is the workaround published as part of 0.34.4 or is it still pending to be published?

vighnesh153 added a commit to vighnesh153/vighnesh153-monorepo that referenced this issue Sep 10, 2023
@deot
Copy link
Contributor

deot commented Sep 11, 2023

Temporarily fix for node environment:

// @vitest-environment node

@shunjizhan
Copy link

shunjizhan commented Sep 11, 2023

got the same issue with vitest 0.34.3, prev versions worked as expected.

For the below function (source), isU8a(someUint8Array) returns false, while it should return ture

export function isU8a (value?: unknown): value is Uint8Array {
  // here we defer the instanceof check which is actually slightly
  // slower than just checking the constrctor (direct instances)
  return (
    ((value && (value as Uint8Array).constructor) === Uint8Array) ||
    value instanceof Uint8Array
  );
}

@vighnesh153
Copy link

Is it not covered in the specs? u8a.spec.ts

@shunjizhan
Copy link

Is it not covered in the specs? u8a.spec.ts

it is covered, but vitest 0.34.3 has this bug that causes it to fail, the isU8a method itself is fine

@7rulnik
Copy link
Contributor

7rulnik commented Sep 22, 2023

@sheremet-va btw what is the correct way to use AbortController, TextEncoder and others within experimentalVmThreads and jsdom/happy-dom environment?

@sheremet-va
Copy link
Member

btw what is the correct way to use AbortController, TextEncoder and others within experimentalVmThreads and jsdom/happy-dom environment?

Environments (jsdom/happy-dom) should provide these APIs.

@7rulnik
Copy link
Contributor

7rulnik commented Sep 22, 2023

@sheremet-va and as I understand it's impossible to transfer (assign it to window/global in setup file) classes like AbortController or TextEncoder to VM threads. So they should be polyfilled with things that exist in VM threads.
Am I right?

@sheremet-va
Copy link
Member

sheremet-va commented Oct 1, 2023

and as I understand it's impossible to transfer (assign it to window/global in setup file) classes like AbortController or TextEncoder to VM threads

It is possible to pass down implementations to VM threads, but some APIs will not accept them, while others will.

For example, if we use node.Uint8Array, then its .buffer will be node.ArrayBuffer, but we already define global ArrayBuffer from jsdom, so there will be a mismatch. Ok, let's then assign global ArrayBuffer to Node's. In this case JSDOM API that returns a buffer, will return JSDOM buffer and instanceof checks will fail:

const blob = new Blob(['Hello'], { type: 'text/plain' })

const arraybuffer = await new Promise<ArrayBuffer>((resolve, reject) => {
  const reader = new FileReader()
  reader.onload = () => resolve(reader.result as ArrayBuffer)
  reader.onerror = () => reject(reader.error)
  reader.readAsArrayBuffer(blob)
})

expect(arraybuffer instanceof ArrayBuffer).toBeTruthy() // fails, because ArrayBuffer is node.ArrayBuffer, but FileReader resolves to jsdom.ArrayBuffer

If we use jsdom.Uint8Array, then Node.js API that checks for it will fail (see #4175). So even if JSDOM implements TextEncoder, most Node.js APIs will reject JSDOM's Uint8Array.

I am not sure where the middle ground is, and open to suggestions.

@sheremet-va
Copy link
Member

Also, looks like happy-dom doesn't have these issues.

@7rulnik
Copy link
Contributor

7rulnik commented Oct 1, 2023

Oh, yeah, I remember this infamous jest issue jestjs/jest#2549
Thank you for the detailed description!

And yeah, it feels like the latest happy-dom has many globals. At least, Request, AbortController, AbortSignal. And it's easy to polyfill TextEncoder/TextDecoder via https://github.com/samthor/fast-text-encoding

@sheremet-va sheremet-va added discussion p2-to-be-discussed Enhancement under consideration (priority) and removed pending triage labels Oct 2, 2023
@smcstewart
Copy link

smcstewart commented Jan 21, 2024

Temporarily fix for node environment:

// @vitest-environment node

Super simple workaround that actually works. Thanks! Put it at the top of your test spec file.

@kyledetella
Copy link

I was banging my head against the wall for a while on this one. None of the solutions I encountered here (or in the JSDom, Jest, or ESBuild repos) were working for my use case. Ultimately, what I ended up doing to get this fixed was to introduce a shim of TextEncoder lifted from the text-encoder library.

First, in my vite.config.ts file:

// vite.config.ts
import "./setupTestEnvironmentCompatibility"; // This MUST be the first import
// ...

And then the setupTestEnvironmentCompatibility file looks like this:

// Only overwrite this if we are in test mode. This is an env var we happen to have in our repo
if (process.env.VITE_IS_TEST === "true") {
  class ESBuildAndJSDOMCompatibleTextEncoder extends TextEncoder {
    constructor() {
      super();
    }

    encode(input: string) {
      if (typeof input !== "string") {
        throw new TypeError("`input` must be a string");
      }

      const decodedURI = decodeURIComponent(encodeURIComponent(input));
      const arr = new Uint8Array(decodedURI.length);
      const chars = decodedURI.split("");
      for (let i = 0; i < chars.length; i++) {
        arr[i] = decodedURI[i].charCodeAt(0);
      }
      return arr;
    }
  }

  Object.defineProperty(global, "TextEncoder", {
    value: ESBuildAndJSDOMCompatibleTextEncoder,
    writable: true,
  });
}

Hope this helps others.

@JoshuaCWebDeveloper
Copy link

Following @kyledetella's workaround, putting the import:

import "./setupTestEnvironmentCompatibility";

in my vite.config.ts file did not work; however, putting it at the top of every *.spec.ts file that was giving me the error (in my case only 2) did work.

@sheremet-va sheremet-va added p2-edge-case Bug, but has workaround or limited in scope (priority) and removed discussion p2-to-be-discussed Enhancement under consideration (priority) labels Jul 2, 2024
@eduardhasanaj
Copy link

eduardhasanaj commented Sep 30, 2024

@sheremet-va What is the status of this issue?
I tried @kyledetella way but could fix it: I got same error.

@KholdStare
Copy link

@eduardhasanaj I am not sure why @kyledetella inserted the code into vite.config.ts directly - that may be a problem for you.

Since this issue is with the JSDom environment and vitest, I put a slightly simplified version of @kyledetella's code in my setupTests.ts which gets run by vitest when it starts the tests.

// JSDom + Vitest don't play well with each other. Long story short - default
// TextEncoder produces Uint8Array objects that are _different_ from the global
// Uint8Array objects, so some functions that compare their types explode.
// https://github.com/vitest-dev/vitest/issues/4043#issuecomment-1905172846
class ESBuildAndJSDOMCompatibleTextEncoder extends TextEncoder {
  constructor() {
    super();
  }

  encode(input: string) {
    if (typeof input !== "string") {
      throw new TypeError("`input` must be a string");
    }

    const decodedURI = decodeURIComponent(encodeURIComponent(input));
    const arr = new Uint8Array(decodedURI.length);
    const chars = decodedURI.split("");
    for (let i = 0; i < chars.length; i++) {
      arr[i] = decodedURI[i].charCodeAt(0);
    }
    return arr;
  }
}

global.TextEncoder = ESBuildAndJSDOMCompatibleTextEncoder;

In vite.config.ts config:

{
  // ...
  test: {
    setupFiles: ["src/setupTests.ts"],
  },
}

@eduardhasanaj
Copy link

@KholdStare Thank you very much for you time. It worked!
I spent a lot of time searching for a callback where we could polyfill correctly but missed setup files option.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
p2-edge-case Bug, but has workaround or limited in scope (priority)
Projects
None yet
Development

No branches or pull requests