Skip to content

feat(error): Decouple public error codes, modularize error files, and enable declaration merging#3127

Open
jonathanedey wants to merge 22 commits intoerror-revampfrom
je-error-re2
Open

feat(error): Decouple public error codes, modularize error files, and enable declaration merging#3127
jonathanedey wants to merge 22 commits intoerror-revampfrom
je-error-re2

Conversation

@jonathanedey
Copy link
Copy Markdown
Collaborator

@jonathanedey jonathanedey commented May 4, 2026

Decouple client-facing error codes from internal SDK details and refactor error handling into dedicated, modular error.ts files, utilizing declaration merging to improve IDE autocomplete and indexing.

  • Merged all service-level error code union types and runtime constant lookups using TypeScript declaration merging to support both runtime lookup and compile-time type safety.
  • Decoupled public client error codes from internal message mappings to protect SDK internals and improve type validation.
  • Modularized all error definitions by extracting service-specific error classes, codes, and catalogs into dedicated error.ts files.
  • Renamed AppErrorCodes to singular AppErrorCode globally across all modules, dependencies, and test suites to maintain strict naming consistency.
  • Configured a targeted ESLint naming filter in .eslintrc.js to protect standard camelCase variable rules while allowing PascalCase for exported error code declarations.

@jonathanedey jonathanedey added the release:stage Stage a release candidate label May 4, 2026
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 refactors error handling by moving error definitions and classes from a central utility file into module-specific error files across the SDK, including Auth, Messaging, and App Check. This involves updating imports and usage throughout the codebase and tests. The review feedback identifies a typo in a reCAPTCHA error code and suggests renaming a mapping function in the token generator to follow standard naming conventions.

Comment thread src/auth/error.ts Outdated
| 'user-not-disabled'
| 'invalid-recaptcha-action'
| 'invalid-recaptcha-enforcement-state'
| 'racaptcha-not-enabled';
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

medium

There seems to be a typo in this error code. racaptcha-not-enabled should likely be recaptcha-not-enabled to be consistent with the other reCAPTCHA-related error codes and the original constant name RECAPTCHA_NOT_ENABLED. This typo appears in a few other places in this file as well.

Suggested change
| 'racaptcha-not-enabled';
| 'recaptcha-not-enabled';

Comment thread src/auth/token-generator.ts Outdated
@jonathanedey jonathanedey changed the title feat: Decouple exported error codes and messages and refactor internal error logic feat(error): Decouple public error codes, modularize error files, and enable declaration merging May 5, 2026
@jonathanedey
Copy link
Copy Markdown
Collaborator Author

/gemini review

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 performs a major refactoring of error handling across the Firebase Admin Node.js SDK by moving service-specific error codes and classes into dedicated error.ts files within each service's directory. This change improves modularity and updates the API surface to use const objects for error codes. The review feedback highlights several opportunities to improve consistency by ensuring that error classes for Database, Firestore, Installations, and InstanceId services extend PrefixedFirebaseError instead of the base FirebaseError. Additionally, the reviewer noted a documentation redundancy in the Extensions service and identified a potential data loss issue in the toJSON implementation of FirebaseMessagingSessionError.

Comment thread src/database/error.ts Outdated
Comment thread src/database/error.ts Outdated
Comment thread src/extensions/error.ts Outdated
Comment thread src/firestore/error.ts Outdated
Comment thread src/firestore/error.ts Outdated
Comment thread src/installations/error.ts Outdated
Comment thread src/installations/error.ts Outdated
Comment thread src/instance-id/error.ts Outdated
Comment thread src/instance-id/error.ts Outdated
Comment thread src/messaging/error.ts
@jonathanedey
Copy link
Copy Markdown
Collaborator Author

/gemini review

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 refactors error handling across the SDK by centralizing error code definitions and error classes into service-specific files. It moves error-related logic out of the internal API client files and into new error.ts files for each service (e.g., Auth, Messaging, Data Connect, etc.). The changes include a breaking fix for a typo in the RECAPTCHA_NOT_ENABLED error code, a simplification of the toJSON method in FirebaseMessagingSessionError, and the removal of explicit __proto__ assignments in error classes. These changes improve modularity and maintainability of the error handling system.

I am having trouble creating individual review comments. Click here to see my feedback.

src/auth/error.ts (119)

high

The error code string has been changed from racaptcha-not-enabled to recaptcha-not-enabled. While this fixes a typo, it is a breaking change for any client-side code or monitoring tools that rely on the exact string value of the error code. Please ensure this breaking change is intentional and documented.

src/messaging/error.ts (312-316)

medium

The toJSON method in FirebaseMessagingSessionError has been simplified to use super.toJSON(). However, the previous implementation explicitly included code and message. While super.toJSON() (from FirebaseError) should include these, it's worth verifying that the resulting JSON structure remains compatible with existing error reporting systems.

src/phone-number-verification/phone-number-verification-api-client-internal.ts (68)

medium

The explicit setting of __proto__ was removed in the refactoring. While modern environments often handle this automatically when extending Error, it was originally added to support older environments or specific TypeScript compilation targets. Ensure that removing this doesn't break instanceof checks for FirebasePhoneNumberVerificationError in all supported environments.

@jonathanedey jonathanedey marked this pull request as ready for review May 6, 2026 20:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

release:stage Stage a release candidate

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants