lib: disable default memory leak warning for AbortSignal

[phryneas]

This change sets the default kMaxEventTargetListeners property for AbortSignal instances to 0, disabling the check per default, to enable users to write isomorphic library code.
If desirable, the max event target listeners check can still be enabled for individual AbortSignal instances by calling setMaxListeners on them.

Refs: #54758

I ran all the test I believe are relevant locally, but I can't run the full test suite as I'm working on a managed Mac and can't change firewall rules, so the test suite just explodes on me with popups.

[phryneas]

(I'm not allowed to add the notable-change label, so someone else will have to do that)

[benjamingr]

@jasnell wdyt?

[jasnell]

While I prefer having the warning in place, I won't block and the code change LGTM

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 88.00%. Comparing base (fe1dd26) to head (d0579b6).
Report is 167 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main   #55816      +/-   ##
==========================================
+ Coverage   87.92%   88.00%   +0.08%     
==========================================
  Files         654      656       +2     
  Lines      187815   188984    +1169     
  Branches    35830    35997     +167     
==========================================
+ Hits       165139   166323    +1184     
+ Misses      15878    15832      -46     
- Partials     6798     6829      +31     

Files with missing lines	Coverage Δ
lib/internal/abort_controller.js	98.06% <100.00%> (+<0.01%)	⬆️

... and 78 files with indirect coverage changes

[jazelly]

The commit message should be under subsystem lib

[phryneas]

@jazelly what's the correct course of action here?
Amend the commit, rewrite the existing commit title and force push?
I'm afraid that that will void all the commit approvals 😅

[jazelly]

what's the correct course of action here?
Amend the commit, rewrite the existing commit title and force push?

Yes, the commit needs to be amended to pass the GHA, so that we can run CI.

I'm afraid that that will void all the commit approvals.

Technically, you just need one more approval / request re-review from the approvers after force pushing.

[phryneas]

@jazelly Okay, thank you for clarifying - I've adjusted the commit message!

Apart from the commit message, the commits are identical

[jazelly]

The first-line commit message needs to be less than 72 characters. Can you please update the commit?

More detailed guideline can be found here. Sorry, probably should've link that in my previous message.

[phryneas]

I followed that guide 😓

The first line should:

contain a short description of the change (preferably 50 characters or less, and no more than 72 characters)

✅ description is 51 characters, plus the lib: prefix

be entirely in lowercase with the exception of proper nouns, acronyms, and the words that refer to code, like function/variable names

✅ everything lowercase except for AbortController

be prefixed with the name of the changed subsystem and start with an imperative verb. Check the output of git log --oneline files/you/changed to find out what subsystems your changes touch.

From the suffix:

lib/*.js (assert, buffer, etc.)

which is why I came up with internal in the first place, because this is in lib/internal.

---

Looking at this, it seems I failed on the subsequent lines - one line is 76 chars long. I probably trusted my local editor which warns at 80.

Fixing...

[phryneas]

@jazelly I've run the check locally via

git rev-parse HEAD~0 | xargs npx -q core-validate-commit --no-validate-metadata --tap

and it seems to pass now.

[nodejs-github-bot]

CI: https://ci.nodejs.org/job/node-test-pull-request/63867/

[nodejs-github-bot]

CI: https://ci.nodejs.org/job/node-test-pull-request/63878/ 💛

[nodejs-github-bot]

Landed in c1ccade