Open phryneas opened 2 weeks ago
The right avenue to purse this change is WHATWG (the group maintaining those standards). From past experience, any time we deviate from the standard we cause more harm than good.
But isn't maxEventTargetListeners
in itself - a limit on the number of listeners and issuing a warning - already a node-only deviation from the standard?
So this would try to explore a way to get closer to the standard, not further away.
By that logic, another alternative suggestion here could be:
AbortController
On all newly created AbortSignal
instances, set the kMaxEventTargetListeners
to 0 by default to align with standard behaviour - users that want a limit on event listeners can still use setMaxListeners
to set a specific limit.
In the research before opening this ticket, I've seen a lot of instances where people were getting this warning with undici fetch
, so this seems to be a somewhat common problem.
In my case, this is ironically caused because I want to pass an AbortSignal
as the signal
option to a lot of addEventListener
calls so I can make sure to clean them up.
Either way, it seems that AbortSignal
is just made to be reused by many fetch
calls or addEventListener
calls, so the current limit of 10 is easily reached by normal intended usage (and afaik not to spec).
@jasnell you added the warning in https://github.com/nodejs/node/pull/36001
@KhafraDev you added those getters/setters in https://github.com/nodejs/node/pull/47039
What do you folks think?
node shouldn't have implemented warnings or interop with EventEmitter in the first place, but we absolutely shouldn't deviate further from the spec and other implementations
@nodejs/web-standards what do you all think?
Pinging @lucacasonato too (hope you don't mind).
Agree with @KhafraDev. If anything this should be a setter function you import from node:events
that you pass the signal to as the first argument.
I agree. We should not extend the standard API with non-standard extensions.
+1 (no nonstandard extensions). I do like the alternative suggestion tho (https://github.com/nodejs/node/issues/54758#issuecomment-2328459853)
I agree with the alternative suggestion as well
@jasnell are you ok for https://github.com/nodejs/node/issues/54758#issuecomment-2328459853?
Just so we're clear - we are within our rights from the spec's PoV to emit a warning on more than N listeners (or really anything, warnings aren't considered observable). We've clarified that several times with WHATWG explicitly to make sure.
I actually prefer that warning remain in place as it has actually proved useful in a couple of cases. But I won't block removing it.
What is the problem this feature will solve?
Right now, there is no stable way to change the
maxEventTargetListeners
of anAbortSignal
without importing fromnode:events
.This makes it impossible to write isomorphic library code that adds more than the default 10 event listeners, without having to resort to workarounds such as
This relies on the
events.maxEventTargetListeners
symbol description, which, to my knowledge is not part of any public API and could change at any time.What is the feature you are proposing to solve the problem?
Add a
AbortSignal.setMaxListeners
instance method so isomorphic code could do something likeWhat alternatives have you considered?
Alternatively, if this is not an option as it could pollute a spec-defined object with additional properties, use
Symbol.for
instead ofnew Symbol
forkMaxEventTargetListeners
and make that a part of the official api.I would be willing to implement this feature.