Firebase error handling and native logs

[gomorizsolt]

As a result of https://gitlab.com/c-hive/payment-gateway/merge_requests/16#note_207430633.

Extracted:

When we use a generic error hadnling in FB it registers the handler as succeeded (besides also logging the error of course). E.g. something().catch(err => { console.error(err); });

[thisismydesign]

Comment by @gomorizsolt moved here:

Please add this to the firebase best practices.

Please consider the following samples before we agree upon this error handling stuff.

When the Promise is rejected because of Promise.reject(...).

static createCustomer() {
  return new Promise((resolve, reject) => {
    return reject({
      error: "CREATE_CUSTOMER_ERROR",
    });
  });
}

// 1. When the error is NOT caught.
function handler(user) {
  return Model.create(user)
    .then(() => Service.createCustomer(user));
}

// Firebase logs rejections by default.
// As it is not wrapped into an error class(i.e. `return reject(...)`), it does not include the stack trace. That is why the output is not [object Object].
// => { error: 'CREATE_CUSTOMER_ERROR' }

// 2. When the error is caught and also re-thrown.
function handler(user) {
  return Model.create(user)
    .then() => Service.createCustomer(user))
    .catch(err => {
      // It does not matter if the error is serialized or not, the log remains the same in both cases => because of `Promise.reject(...)`.
      console.error(err);
      // OR
      console.error(JSON.stringify(err));
      // => { error: 'CREATE_CUSTOMER_ERROR' }

      // Re-throw the error to reflect the Promise's state to Firebase.
      // This adds an additional log entry to Firebase's console without serializing the object out of the box.
      throw new Error(err);
      // => Error: [object Object] at Promise (/srv/app/services/chargebee-service.js:27:13) at new Promise ...

      // Thereby, the `err` must be serialized by calling JSON.stringfy().
      // Also adds an additional log entry to Firebase's console but with a readable error object.
      throw new Error(JSON.stringify(err));
      // => Error: {"error":"CREATE_CUSTOMER_ERROR"} at userModel.create.then.catch.err (/srv/app/firebase-handlers/signup/signup.js:13:13) at <anonymous>
    });
}

When the Promise is rejected because of throw new Error(...).

static createCustomer() {
  return new Promise((resolve, reject) => {
    throw new Error({
      error: "CREATE_CUSTOMER_ERROR",
    });
  });
}

// 1. When the error is NOT caught.
function handler(user) {
  return Model.create(user)
    .then(() => Service.createCustomer(user));
}

// Firebase logs the error to the console along with its stack trace, but does not serialize the object(i.e. the error).
// => Error: [object Object] at Promise (/srv/app/services/chargebee-service.js:27:13) at new Promise ...

// 2. When the error is caught.
function handler(user) {
  return Model.create(user)
    .then(() => Service.createCustomer(user))
    .catch(err => {
      // => Serializing is required as it contains the stack trace along with the error itself.
      console.error(JSON.stringify(err));

      // Re-throw the error to reflect the Promise's state to Firebase.
      throw new Error(JSON.stringify(err));
      // Results in the same behavior.
    });
}

[thisismydesign]

Instead of throw new Error(err); why not throw err;? IMO we should not fiddle with the error object unless needed.

As per what is being logged: I would not rely on FB's logging because as you pointed out it's not always consistent and requires manual intervention. I'd follow our existing error handling practices for logging the error (i.e. JSON.stringify). My only concern here is that I want the handler to register as failed in FB. This requires us to let the promise fail which we should do. FB will create another (perhaps unneeded) log entry about the error but that's just a side effect, we should not care about it imo as we have our own meaningful logs.

[gomorizsolt]

Instead of throw new Error(err); why not throw err;?

Thanks for the idea, I have completely forgotten this approach. It is suffice to throw the error without wrapping it into a class. I am going to update the descriptions accordingly.