feat(error): Decouple public error codes, modularize error files, and enable declaration merging#3127
feat(error): Decouple public error codes, modularize error files, and enable declaration merging#3127jonathanedey wants to merge 22 commits intoerror-revampfrom
Conversation
There was a problem hiding this comment.
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.
| | 'user-not-disabled' | ||
| | 'invalid-recaptcha-action' | ||
| | 'invalid-recaptcha-enforcement-state' | ||
| | 'racaptcha-not-enabled'; |
There was a problem hiding this comment.
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.
| | 'racaptcha-not-enabled'; | |
| | 'recaptcha-not-enabled'; |
…ong side error code type
|
/gemini review |
There was a problem hiding this comment.
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.
|
/gemini review |
There was a problem hiding this comment.
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)
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)
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)
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.
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.
error.tsfiles.AppErrorCodesto singularAppErrorCodeglobally across all modules, dependencies, and test suites to maintain strict naming consistency..eslintrc.jsto protect standardcamelCasevariable rules while allowingPascalCasefor exported error code declarations.