Firebase-admin-node: FirebaseError type definition as class instead of interface

[vlapo]

Description

Currently is FirebaseError defined as interface in typescript definition file. So in case of async/await we have to handle errors checking e.g. code property existence:

try {
    const decodedIdToken = await this.auth.verifyIdToken(firebaseIdToken);
    // some another calls async methods
} catch(err) {
    if (err.code === 'auth/argument-error') {
        // handle errors - but err is still any type
    }
}

This is proper way to handle errors but a little bit cleaner way would be:

try {
    const decodedIdToken = await this.auth.verifyIdToken(firebaseIdToken);
    // some another calls async methods
} catch(err) {
    if (err instanceof FirebaseError) {
        // handle error codes and proper typing here
    }
}

It is just a small improvement but in this way we have correct type in if statement and also FirebaseError.code would be some string literal type with defined all possible error codes.

type FirebaseErrorCode = 'auth/argument-error' | 'auth/id-token-expired' | ... ;

class FirebaseError extends Error {
    code: FirebaseErrorCode;
}

I think this could be really helpful in error handling.

Target environment

• Operating System version: all
• Firebase SDK version: latest
• Library version: latest
• Firebase Product: common

[google-oss-bot]

I found a few problems with this issue:

• I couldn't figure out how to label this issue, so I've labeled it for a human to triage. Hang tight.
• This issue does not seem to follow the issue template. Make sure you provide all the required information.

[vlapo]

Also using enum with comments instead of string literal could be a better improvement.

enum FirebaseErrorCode {
    /** Thrown if a method is called with incorrect arguments. */
    AUTH_ARGUMENT_ERROR = 'auth/argument-error',
    // ...
}

And comments are visible in IDE:

[hiranya911]

I think we can expose FirebaseError as a class. But I'm not sure about error codes. We have way too many error codes, and we've been trying to find a way to phase them out entirely, and go for a different way of error handling.

[hiranya911]

I've looked into this a bit, and it turns out to be more complicated than I originally imagined. Not only FirebaseError is defined as an interface, it is not really exported by the FirebaseNamespace. Therefore this cannot be implemented as a simple change to our typings.

it.only('supports instanceOf checks', () => {
    try {
      admin.app('non.existing');
    } catch (err) {
      expect(err.code).to.equal('app/no-app');
      expect(err).to.be.instanceOf(admin.FirebaseError);
    }
});

This results in:

supports instanceOf checks:
     TypeError: Cannot read property 'name' of undefined
      at Object.module.exports [as getName] (node_modules/chai/lib/chai/utils/getName.js:18:12)
      at Assertion.assertInstanceOf (node_modules/chai/lib/chai/core/assertions.js:794:18)
      at Assertion.ctx.(anonymous function) (node_modules/chai/lib/chai/utils/addMethod.js:41:25)
      at doAsserterAsyncAndAddThen (node_modules/chai-as-promised/lib/chai-as-promised.js:293:29)
      at Assertion.<anonymous> (node_modules/chai-as-promised/lib/chai-as-promised.js:252:17)
      at Assertion.ctx.(anonymous function) [as instanceOf] (node_modules/chai/lib/chai/utils/overwriteMethod.js:49:33)
      at Context.<anonymous> (test/integration/app.spec.ts:100:25)

To make this work, we need something like the following:

// firebase-namespace.ts
import {FirebaseError} from './utils/error';

/**
 * Global Firebase context object.
 */
export class FirebaseNamespace {
    public credential = firebaseCredential;
    public SDK_VERSION = '<XXX_SDK_VERSION_XXX>';
    public INTERNAL: FirebaseNamespaceInternals;
    public FirebaseError = FirebaseError;

[vinayuttam]

For an expired token the following is the error message:

{ code: 'auth/argument-error',
  message:
   'Firebase ID token has expired. Get a fresh token from your client app and try again (auth/id-token-expired). See https://firebase.google.com/docs/auth/admin/verify-id-tokens for details on how to retrieve an ID token.' }

Shouldn't the code be auth/id-token-expired instead of an auth/argument-error' as per the documentation[1]?

[1] - https://firebase.google.com/docs/auth/admin/errors

cc: @hiranya911 @vlapo

[vlapo]

@vinayuttam https://github.com/firebase/firebase-admin-node/issues/179

[vinayuttam]

@vlapo thanks for pointing to that issue. I've missed it some how.

[hiranya911]

I'm working on a proposal to revamp our error handling in general. I think we should expose our errors as classes instead of interfaces as part of this. Will keep this thread posted.

[driescroons]

Any update on this @hiranya911? I'm currently checking if my error object has a code property in my error middleware, but this interferes with other packages I'm using. (other errors thinking it's a FirebaseError by having a code property)

It would be a whole lot cleaner / easier if we could use error instanceof FirebaseError. The codes on the error object work perfectly fine for me, as I can do the label mapping to a nicer message myself, but the problem is more in the interface not being a class.

Edit: I just noticed the errors in auth do have nice labels for mapping, but the firestore ones have just a number.

[hiranya911]

So far the new error handling model has been released in Python and .NET. We plan to bring it to Java soon. I think we can look into implementing it in Node.js towards the 2nd half of 2020.

[driescroons]

Would you mind sharing a link of the node sdk roadmap? Cannot seem to find it 😀

[hiranya911]

@driescroons we don't publish our roadmaps publicly.

[angelhodar]

Just in case anyone is interesed, instead of using instanceof to check if error comes from Firebase, I have made this workaround to check if error is from the auth system:

function isFirebaseError(err) {
  if (err.code)
    return err.code.startsWith('auth/');
  return false;
}

Hope it helps ;)

[jefersonla]

Any update on this @hiranya911?

[hiranya911]

Not yet. We have some other high priority work scheduled for the first half of this year. We can revisit this once that's done.

[abierbaum]

Here is a method I am using for now that uses a custom type guard.

interface IFirebaseError extends Error {
   code: string;
   message: string;
   stack?: string;
}

/** Check if we have a firebase error. */
export function isFirebaseError(err: any): err is IFirebaseError {
   return err.code && err.code.startsWith('auth/');
}