refactor: remove utils.allSettled polyfill in favor of native Promise.allSettled#10348
Open
kyungseopk1m wants to merge 2 commits intofirebase:mainfrom
Open
refactor: remove utils.allSettled polyfill in favor of native Promise.allSettled#10348kyungseopk1m wants to merge 2 commits intofirebase:mainfrom
kyungseopk1m wants to merge 2 commits intofirebase:mainfrom
Conversation
Contributor
There was a problem hiding this comment.
Code Review
This pull request replaces the custom utils.allSettled polyfill with the native Promise.allSettled implementation across several files, including the fabricator, reporter, and emulator components, and removes the now-obsolete tests and utility definitions. A review comment identifies that the removal in src/utils.ts left behind dangling comments and ESLint directives, and suggests refactoring or unifying another similar implementation (promiseAllSettled) still present in the file to avoid duplication.
joehan
approved these changes
Apr 15, 2026
….allSettled The custom allSettled polyfill in src/utils.ts was added to support Node versions below 12.9.0, as noted by the TODO comment at the time of introduction. The minimum Node.js version requirement is now >=20.0.0 (package.json engines field, enforced at runtime in src/bin/firebase.ts), which far exceeds the 12.9.0 threshold. This commit removes the polyfill (allSettled function and the associated PromiseFulfilledResult, PromiseRejectedResult, PromiseResult types) and replaces all call sites with the native Promise.allSettled. The PromiseRejectedResult cast in fabricator.ts now resolves against the TypeScript standard lib type instead of the local definition.
…directive Remove the remaining custom promiseAllSettled function (SettledPromiseResolved, SettledPromiseRejected, SettledPromise types) which duplicates the same pattern as the allSettled polyfill removed in the previous commit. The function had no production call sites and its test block verified only the custom implementation. Also remove the dangling JSDoc comment and eslint-disable directive left over from the previous deletion.
f02cfef to
69cfb1b
Compare
Author
|
removed CHANGELOG entry and resolved conflicts with main. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
Removes the custom
allSettledpolyfill fromsrc/utils.tsand replaces all call sites with the nativePromise.allSettled.Motivation
The polyfill was introduced to support Node.js versions below 12.9.0, where
Promise.allSettledwas not yet available. The source comment made this intent explicit:// TODO: delete once min Node version is 12.9.0 or greaterThe minimum Node.js version requirement is now >=20.0.0, as declared in
package.json(enginesfield) and enforced at runtime insrc/bin/firebase.ts. This far exceeds the 12.9.0 threshold, making the polyfill dead code.This is the same pattern as several recent cleanups in the repo (e.g.
cloneDeepTODO comments also targeting a minimum-version gate).Changes
PromiseFulfilledResult,PromiseRejectedResult,PromiseResulttype exports andallSettledfunction fromsrc/utils.tssrc/utils.spec.tsPromise.allSettleddirectly:src/emulator/functionsEmulator.tssrc/functions/secrets.tssrc/commands/ext-dev-deprecate.tssrc/commands/ext-dev-undeprecate.tssrc/deploy/functions/validate.tssrc/deploy/functions/release/fabricator.ts(×3 call sites)src/deploy/functions/release/reporter.tsPromiseRejectedResultcast infabricator.tsnow resolves against the TypeScript standard lib type rather than the removed local definition — no behavior changeTesting
npx tsc --noEmitpasses with no errors