Skip to content

refactor: remove utils.allSettled polyfill in favor of native Promise.allSettled#10348

Open
kyungseopk1m wants to merge 2 commits intofirebase:mainfrom
kyungseopk1m:refactor/remove-allsettled-polyfill
Open

refactor: remove utils.allSettled polyfill in favor of native Promise.allSettled#10348
kyungseopk1m wants to merge 2 commits intofirebase:mainfrom
kyungseopk1m:refactor/remove-allsettled-polyfill

Conversation

@kyungseopk1m
Copy link
Copy Markdown

Description

Removes the custom allSettled polyfill from src/utils.ts and replaces all call sites with the native Promise.allSettled.

Motivation

The polyfill was introduced to support Node.js versions below 12.9.0, where Promise.allSettled was not yet available. The source comment made this intent explicit:

// TODO: delete once min Node version is 12.9.0 or greater

The minimum Node.js version requirement is now >=20.0.0, as declared in package.json (engines field) and enforced at runtime in src/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. cloneDeep TODO comments also targeting a minimum-version gate).

Changes

  • Removed PromiseFulfilledResult, PromiseRejectedResult, PromiseResult type exports and allSettled function from src/utils.ts
  • Removed corresponding unit tests for the polyfill from src/utils.spec.ts
  • Updated all 8 call sites across the codebase to use Promise.allSettled directly:
    • src/emulator/functionsEmulator.ts
    • src/functions/secrets.ts
    • src/commands/ext-dev-deprecate.ts
    • src/commands/ext-dev-undeprecate.ts
    • src/deploy/functions/validate.ts
    • src/deploy/functions/release/fabricator.ts (×3 call sites)
    • src/deploy/functions/release/reporter.ts
  • The PromiseRejectedResult cast in fabricator.ts now resolves against the TypeScript standard lib type rather than the removed local definition — no behavior change

Testing

  • npx tsc --noEmit passes with no errors
  • Existing tests unaffected; the removed test block covered only the polyfill's own implementation, not any business logic

Copy link
Copy Markdown
Contributor

@gemini-code-assist gemini-code-assist bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Comment thread src/utils.ts
Comment thread CHANGELOG.md Outdated
….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.
@kyungseopk1m kyungseopk1m force-pushed the refactor/remove-allsettled-polyfill branch from f02cfef to 69cfb1b Compare April 16, 2026 05:04
@kyungseopk1m
Copy link
Copy Markdown
Author

removed CHANGELOG entry and resolved conflicts with main.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants