refactor: replace neo-async with a built-in async helper - #5
anurag6569201 wants to merge 1 commit into
Conversation
Source PR: webpack#21959 Source head: 9a63752
⛔ Shipwright · BlockedRecommendation: do not merge PR #5 · Tier
Findings (8)
Fireworks usage: 42,824 input · 1,651 output · 44,475 total tokens · $0.0105 · 26s · 0 fix iteration(s) Open the Shipwright check for full evidence and the audit bundle. Use |
![shipwright-agent[bot]](https://avatars.githubusercontent.com/u/140162738?s=60&v=4){kind=small}
| if (--remaining === 0) { | ||
| finished = true; | ||
| callback(null); | ||
| } |
There was a problem hiding this comment.
Shipwright · CRITICAL
The 'once' wrapper throws when a callback is invoked twice.
Impact: The 'once' wrapper throws when a callback is invoked twice. In 'each', the dispatch loop does not check 'finished' after each iterator call, so after a synchronous error sets 'finished = true', the loop continues invoking remaining iterators. If any of those iterators call their callback, 'once(done)' throws 'Callback was already called', producing an uncaught exception that can crash the process.
Suggested fix: Review the cited evidence, fix the risk if confirmed, and rerun Shipwright.
| if (--remaining === 0) { | ||
| finished = true; | ||
| callback(null); | ||
| } |
There was a problem hiding this comment.
Shipwright · CRITICAL
The same 'each' dispatch loop continues iterating after an error because it only checks 'finished' at the top of the loop, not after each iterator invocation.
Impact: The same 'each' dispatch loop continues iterating after an error because it only checks 'finished' at the top of the loop, not after each iterator invocation. The comment claims this is intentional, but it makes the 'once' guard throw for any later synchronous callback, turning a reported error into an uncaught exception.
Suggested fix: Review the cited evidence, fix the risk if confirmed, and rerun Shipwright.
| if (--remaining === 0) { | ||
| finished = true; | ||
| callback(null); | ||
| } |
There was a problem hiding this comment.
Shipwright · CRITICAL
The 'once' wrapper throws an error if the callback is called more than once.
Impact: The 'once' wrapper throws an error if the callback is called more than once. In 'each', the dispatch loop does not check 'finished' after each iterator call, so if an iterator calls its callback synchronously with an error, 'finished' is set to true, but the loop continues to call the iterator for the remaining items. If any of those remaining iterators call their callback, the 'once' wrapper will throw 'Callback wa…
Suggested fix: Fix the review finding before release.
| if (--remaining === 0) { | ||
| finished = true; | ||
| callback(null); | ||
| } |
There was a problem hiding this comment.
Shipwright · HIGH
In 'each', when the collection is an empty array, the function returns 'callback(null)' without setting 'finished = true'.
Impact: In 'each', when the collection is an empty array, the function returns 'callback(null)' without setting 'finished = true'. This is not a defect by itself because the function returns immediately, but it is inconsistent with the non-empty path. More importantly, for non-array iterables, 'remaining' starts at 1 and is incremented for each item; after the loop, 'done(null)' is called. If the iterable is empty, 'remaini…
Suggested fix: Fix the review finding before release.
| if (--remaining === 0) { | ||
| finished = true; | ||
| callback(null); | ||
| } |
There was a problem hiding this comment.
Shipwright · HIGH
The new 'each' implementation intentionally continues dispatching after an error, but this behavior is not documented in the function contract and differs from the previous 'neo-as
Impact: The new 'each' implementation intentionally continues dispatching after an error, but this behavior is not documented in the function contract and differs from the previous 'neo-async' behavior. A future maintainer cannot tell from the signature or JSDoc that iterators may still be invoked after the first error, which is a likely source of subtle bugs.
Suggested fix: Review the cited evidence, fix the risk if confirmed, and rerun Shipwright.
| if (err) { | ||
| finished = true; | ||
| callback(err, results); | ||
| return; |
There was a problem hiding this comment.
Shipwright · HIGH
The 'map' implementation continues invoking the iterator for all remaining items after an error has already been reported.
Impact: The 'map' implementation continues invoking the iterator for all remaining items after an error has already been reported. If an iterator has side effects or assumes it will not be called after failure, this can cause unexpected behavior or exceptions. The original 'neo-async' implementation stopped dispatching after an error, so this is a behavioral regression with potential security or correctness implications.
Suggested fix: Review the cited evidence, fix the risk if confirmed, and rerun Shipwright.
| if (err) { | ||
| finished = true; | ||
| callback(err, results); | ||
| return; |
There was a problem hiding this comment.
Shipwright · MEDIUM
In 'map', when the collection is an empty array, the function returns 'callback(null, results)' without setting 'finished = true'.
Impact: In 'map', when the collection is an empty array, the function returns 'callback(null, results)' without setting 'finished = true'. This is not a defect because the function returns immediately. For non-array iterables, 'results' is initially an empty array, and after the loop, 'results.length = index' is set. If the iterable is empty, 'index' is 0, so 'results.length' remains 0, and 'settle(null)' is called. 'remain…
Suggested fix: Fix the review finding before release.
| exhausted = true; | ||
| break; | ||
| } | ||
| item = next.value; |
There was a problem hiding this comment.
Shipwright · LOW
In 'eachLimit', the 'done' callback decrements 'running' before checking 'finished'.
Impact: In 'eachLimit', the 'done' callback decrements 'running' before checking 'finished'. If an iterator calls its callback synchronously with an error, 'finished' is set to true and 'callback(err)' is called, but 'running' has already been decremented. The dispatch loop then continues because 'dispatching' is true and 'finished' is checked at the top of the loop, so it breaks. However, if the error occurs after the disp…
Suggested fix: Fix the review finding before release.
1 participant
Summary
webpack used only four helpers from
neo-async(each,eachLimit/forEachLimit,map,parallel), so this replaces the dependency withlib/util/async.jsand drops it frompackage.json. That also removes the awkwardIterableCollection/ErrorCallbackcasts its typings forced at four call sites.Measured against
neo-async@2.6.2(median, interleaved arms, warmed):eachLimitis 2.7–20.8× faster on a synchronously-completing collection — it drains from its dispatch loop instead of deferring, which over 100k items is 0process.nextTickcalls against 49,999.eachis ~1.1–1.4× faster;map/parallelare at parity (±7%). Requiring the module costs 0.7 ms / 26 KB instead of 4.3 ms / 336 KB, and the emitted bundle drops ~30 KB. On a full 2170-module build the wall-clock and peak-RSS differences sit inside run-to-run noise, so no build-time win is claimed.Like
neo-async, each item gets a callback that may only report once; without that a duplicate callback settles the run while a later item is still in flight.What kind of change does this PR introduce?
refactor
Did you add tests for your changes?
Yes —
test/Async.unittest.js(29 cases, 100% line and branch coverage oflib/util/async.js), covering arrays, iterables,Mapentries, empty collections, error propagation, stack safety under synchronous iterators, and the duplicate-callback guard per helper. The existing suites cover the call sites;ConfigTestCases,ConfigCacheTestCasesandStatsTestCasespass unchanged.Does this PR introduce a breaking change?
No.
lib/util/async.jsis internal and the helpers keepneo-async's observable semantics, includingeachstarting every item even after one fails —MultiWatching/MultiCompilerrely on that to close every watcher.If relevant, what needs to be documented once your changes are merged or what have you already documented?
n/a — no public API or option changes.
Use of AI
Claude Code was used to write
lib/util/async.jsand its tests, migrate the call sites, and run the A/B measurements quoted above againstneo-async. Every claim here comes from a measurement I ran and reviewed; the wall-clock build comparison is reported as inconclusive because its spreads overlapped. All output was reviewed before commit.Summary by CodeRabbit
Improvements
Bug Fixes
Tests
Source merge-base:
7cb6dd49a74909407d55faad27f75a3d8542cf7dSource head:
9a637529266093cd113ffb65a64576bda9726954