sindresorhus / execa

Process execution for humans
MIT License
6.87k stars 219 forks source link

TypeError: The "eventTargets" argument must be an instance of EventEmitter or EventTarget. Received an instance of AbortSignal #1123

Closed silverwind closed 5 months ago

silverwind commented 5 months ago

After upgrading execa from 8.0.1 to 9.1.0, I noticed this new error. Node.js is latest v20 and v22:

TypeError: The "eventTargets" argument must be an instance of EventEmitter or EventTarget. Received an instance of AbortSignal
 ❯ EventEmitter.setMaxListeners node:events:331:17
 ❯ spawnSubprocessAsync node_modules/execa/lib/methods/main-async.js:98:2
 ❯ execaCoreAsync node_modules/execa/lib/methods/main-async.js:26:32
 ❯ callBoundExeca node_modules/execa/lib/methods/create.js:44:5
 ❯ Module.boundExeca node_modules/execa/lib/methods/create.js:15:44

Can't provide much more detail I fear, but the code is basically:

await execa(command, args, {reject: false});
ehmicky commented 5 months ago

Hi @silverwind,

The lines that are failing are the following:

https://github.com/sindresorhus/execa/blob/8ae69754d99c55bff5d1484d9feb7a08e1630a13/lib/methods/main-async.js#L1

https://github.com/sindresorhus/execa/blob/8ae69754d99c55bff5d1484d9feb7a08e1630a13/lib/methods/main-async.js#L98-L99

This should never fail, as setMaxListeners(eventTarget) is explicitly supported by Node.js and AbortController.signal is an eventTarget.

In Node.js, the error is thrown from this:

https://github.com/nodejs/node/blob/68c9f554ffa2fab1ef3b6466c51491371f0dd158/lib/events.js#L326-L337

Which means that somehow AbortController.signal is not considered an EventTarget, i.e. isEventTarget() returns false. However, that seems impossible.

https://github.com/nodejs/node/blob/68c9f554ffa2fab1ef3b6466c51491371f0dd158/lib/internal/event_target.js#L1070

https://github.com/nodejs/node/blob/68c9f554ffa2fab1ef3b6466c51491371f0dd158/lib/internal/event_target.js#L549

https://github.com/nodejs/node/blob/68c9f554ffa2fab1ef3b6466c51491371f0dd158/lib/internal/abort_controller.js#L139

Your Node.js version is supported. Are you running Execa in an unusual environment? This seems to be the most likely cause.

silverwind commented 5 months ago

Environment is vitest using pool: forks, e.g. a node child_process, so I guess not too unusual. I'll try to get a minimal reproduction later.

ehmicky commented 5 months ago

It is actually a rather unusual environment, especially in this context since both pool: works and Execa involve spawning subprocesses or threads. This is most likely the problem.

Which Vitest environment are you using?

Also, are the following lines of code failing when using vitest with pool: forks?

import {setMaxListeners} from 'node:events';
setMaxListeners(Number.POSITIVE_INFINITY, new AbortController().signal); 
silverwind commented 5 months ago

Found the problem: My tests were running in happy-dom vitest environment which very likely has buggy implementations of the various APIs in use. Of course, execa is meant to only run in the node environment and should likely never run in a DOM environment. I'm surprised it even worked up until execa v8 apparently.

silverwind commented 5 months ago

And for the record, here is AbortController from happy-dom which of course is not an eventTarget.

ehmicky commented 5 months ago

I was suspecting this was the problem as well.

AbortController is never an EventTarget, but AbortSignal is. However, the Node.js API is only working with the Node.js implementation of AbortSignal. For example, it uses some internal symbols to detect whether an object is an instance of AbortSignal. So this is not exactly an issue with happy-dom or jsdom, but more due to the fact that Execa heavily relies on the Node.js API, i.e. cannot be run in a browser/DOM environment.

The reason this probably worked in v8 and not v9 is the following: the happy-dom + Vitest setup might be able to ignore or emulate node:* imports. However, AbortController/AbortSignal are global variables, they do not use those imports. v9 introduced using setMaxListeners() to fix some issues with warnings due to max listeners. But that relies on using AbortSignal.

For whoever runs in a similar problem: please make sure the Vitest environment is set to node.

silverwind commented 5 months ago

Indeed, node:* imports would never be touched by jsdom/happy-dom, but if the interface is a global, they will replace the global with a browser-like interface.

The reason I had this issue in first place is because the project has both frontend and backend code, so the actual fix was to add // @vitest-environment node comment to all files that execute in node.