Not generally a fan of Boolean args in public APIs. Perhaps having a separate unwrappedListeners() method would work?

I think it would need to be wrappedListeners or something similar. Will think on it.

If you want an option argument, I'd prefer to be explicit and call it unwrapOnceListeners (which could also be a string or object instead of a boolean).

However, from a performance point of view new methods for separate input types should always be a good idea.

This should be addressed now, instead we have a wrappedListeners method on EventEmitter.

Having the -- at the start here just looks odd. Having it at the end may be more readable

Yeah, I'm not sure which way to go here. It matches the rest of the events. Feels more confusing if we go back and forth throughout this code. Maybe just convert all the usage within this file to be postfix (where the result isn't being used)?

I'm okay with having consistency in this file.

It probably looks a lot less weird to you if you're used to writing C++ code :)

If you want an option argument, I'd prefer to be explicit and call it unwrapOnceListeners (which could also be a string or object instead of a boolean).

However, from a performance point of view new methods for separate input types should always be a good idea.

I'm okay with having consistency in this file.

It probably looks a lot less weird to you if you're used to writing C++ code :)

I'm still not sure what exactly shouldCatch entails. I'd like to get #17159 done at some point... maybe we can try to do that before this lands?

This part is slightly different than the work within that PR. It's so we don't need ClearFatalExceptionHandlers which deep reaches into a bunch of JS code (including _events).

The part of this PR that would be made obsolete by #17159 is within lib/domain.js (& a bit within node.cc). I'm guessing this could land and then we can refactor later? Not sure where that work is at though so I'm happy to wait if it's close.

Edit: The main reason I say 'land this' is just because it's strictly better than what's there currently which requires actually inspecting the _events object and looking for error handlers, whereas now that responsibility is mostly in JS land and all we check for is a single flag. Far from ideal but a lot better.

I don't think automagically adjusting the n value is a good idea.

I mean, we don't have a ton of options here. The benchmark doesn't run long enough with 1 listener whereas setting a higher default n makes the other ones run way too long.

That's just a general benchmarking problem, just like any particular n value might cause things to run too fast or too slow on someone else's machine. At some point you will just need to supply your own set of values for each parameter if you have combinations that vary in speed that much.

Missing end parenthesis. Also, maybe link once()?

I think saying 'event named' sounds a little confusing. Maybe something like 'event in' instead? Or maybe just 'event'?

I can change this but we have another 9 instances of this exact phrasing in this doc.

I think the current wording is fine here and if we do change it, I'd rather do that throughout the doc and in a separate PR for consistency

You might check the performance on this, but I remember at least back with Crankshaft, putting properties like this (that are likely to change in every EventEmitter instance) on the prototype could slow things down.

It's definitely fine now from the testing I did. (I ran benchmarks with & without.)

Did you benchmark this particular change? I would think that checking list.warned is less expensive than calling out to $getMaxListeners() every time, when .warned === true?

Accessing .warned or any other named key on an Array is really expensive from the testing I've done. Also, even if it wasn't, it's better to optimize for the case where warned === false since that's going to be more common.

(It's been a while but as I can recall, this change alone made roughly 2% difference for cases where warned === false. This was before I added all the major changes that account for the 20-120% difference.)

I kind of feel like something like rawListeners() might be a better name, but this should be okay.

(i.e. I’m not saying it is a better name, just that wrappedListeners() does not immediately tell you what this does … neither does rawListeners(), obviously, but I think it comes closer? 😄 )

I think the current wording is fine here and if we do change it, I'd rather do that throughout the doc and in a separate PR for consistency

Tiny nit: Can you add {} for the if as well if there is one for the else part as well?

I wonder if it would make sense to turn this into a closure where unwrap can be constant-folded, like:

function makeListeners(unwrap) {
  return function listeners(type) {
    // Code of _listeners with `this` as `target`
  }
}

EventEmitter.prototype.listeners = makeListeners(true);
EventEmitter.prototype.rawListeners = makeListeners(false);

This was just to mimic how we do it elsewhere in this file, namely addListener and prependListener but yeah, this could also work. I suppose the suggested version does close over one variable whereas what I'm guessing happens with the current implementation is that V8 inlines _listeners?

No clue what's preferable. Maybe just stick with consistency for now?

If there is no perf difference, then yes, just stick with this :)

Tiny nit: It would make sense to me if the comment was inside the if since it refers to what must be true if the condition is fulfilled... does that make sense?

This will cause memory leaks.

Yep, I originally ran a modified version of our ee-add-remove benchmark but the 25e4 iterations are not very helpful in testing this. It also regresses the performance once the size of the dictionary object grows (although that seems to not be an issue in later versions of V8, the memory still is).

Mostly I think I was just too optimistic re: user input into the EventEmitter being mostly constant. Probably not a safe assumption.

This will also cause memory leaks.