nodejs
lib: disable default memory leak warning for AbortSignal
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: https://github.com/nodejs/node/issues/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.
(I'm not allowed to add the notable-change label, so someone else will have to do that)
Codecov Report
All modified and coverable lines are covered by tests :white_check_mark:
Project coverage is 88.00%. Comparing base (
fe1dd26) to head (d0579b6). Report is 1510 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%) |
:arrow_up: |
:rocket: New features to boost your workflow:
- :snowflake: Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
- :package: JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.
![codecov[bot] avatar](https://avatars.githubusercontent.com/in/254?v=4 "Published by a pub.dev verified publisher"){kind=small}
@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 😅
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.
@jazelly Okay, thank you for clarifying - I've adjusted the commit message!
Apart from the commit message, the commits are identical
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.
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...
@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.
