Refactor FirebaseError

Author: mmermerkaya

packages/util/index.ts

@@ -30,7 +30,6 @@ export {
   ErrorFactory,
   ErrorList,
   FirebaseError,
-  patchCapture,
   StringLike
 } from './src/errors';
 export { jsonEval, stringify } from './src/json';

packages/util/src/errors.ts

@@ -54,115 +54,95 @@
  *     }
  *   }
  */
-export type ErrorList<T> = { [code: string]: string };
+export type ErrorList<T extends string = string> = {
+  readonly [K in T]: string
+};
 
 const ERROR_NAME = 'FirebaseError';
 
 export interface StringLike {
   toString: () => string;
 }
 
-let captureStackTrace: (obj: Object, fn?: Function) => void = (Error as any)
-  .captureStackTrace;
-
-// Export for faking in tests
-export function patchCapture(captureFake?: any): any {
-  let result: any = captureStackTrace;
-  captureStackTrace = captureFake;
-  return result;
+export interface ErrorData {
+  [key: string]: StringLike | undefined;
 }
 
 export interface FirebaseError {
   // Unique code for error - format is service/error-code-string
-  code: string;
+  readonly code: string;
 
   // Developer-friendly error message.
-  message: string;
+  readonly message: string;
 
   // Always 'FirebaseError'
-  name: string;
+  readonly name: typeof ERROR_NAME;
 
   // Where available - stack backtrace in a string
-  stack: string;
-}
+  readonly stack: string;
 
-export class FirebaseError implements FirebaseError {
-  public stack: string;
-  public name: string;
-
-  constructor(public code: string, public message: string) {
-    let stack: string;
-    // We want the stack value, if implemented by Error
-    if (captureStackTrace) {
-      // Patches this.stack, omitted calls above ErrorFactory#create
-      captureStackTrace(this, ErrorFactory.prototype.create);
-    } else {
-      try {
-        // In case of IE11, stack will be set only after error is raised.
-        // https://docs.microsoft.com/en-us/scripting/javascript/reference/stack-property-error-javascript
-        throw Error.apply(this, arguments);
-      } catch (err) {
-        this.name = ERROR_NAME;
-        // Make non-enumerable getter for the property.
-        Object.defineProperty(this, 'stack', {
-          get: function() {
-            return err.stack;
-          }
-        });
-      }
-    }
-  }
+  // Additional custom error data that was used in the template.
+  readonly data: ErrorData;
 }
 
-// Back-door inheritance
-FirebaseError.prototype = Object.create(Error.prototype) as FirebaseError;
-FirebaseError.prototype.constructor = FirebaseError;
-(FirebaseError.prototype as any).name = ERROR_NAME;
-
-export class ErrorFactory<T extends string> {
-  // Matches {$name}, by default.
-  public pattern = /\{\$([^}]+)}/g;
+// Based on code from:
+// https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Error#Custom_Error_Types
+export class FirebaseError extends Error {
+  readonly name = ERROR_NAME;
 
   constructor(
-    private service: string,
-    private serviceName: string,
-    private errors: ErrorList<T>
+    readonly code: string,
+    message: string,
+    readonly data: ErrorData = {}
   ) {
-    // empty
-  }
+    super(message);
+
+    // Fix For ES5
+    // https://github.com/Microsoft/TypeScript-wiki/blob/master/Breaking-Changes.md#extending-built-ins-like-error-array-and-map-may-no-longer-work
+    Object.setPrototypeOf(this, FirebaseError.prototype);
 
-  create(code: T, data?: { [prop: string]: StringLike }): FirebaseError {
-    if (data === undefined) {
-      data = {};
+    // Maintains proper stack trace for where our error was thrown.
+    // Only available on V8.
+    if (Error.captureStackTrace) {
+      Error.captureStackTrace(this, ErrorFactory.prototype.create);
     }
+  }
+}
 
-    let template = this.errors[code as string];
+export class ErrorFactory<ErrorCode extends string> {
+  constructor(
+    private readonly service: string,
+    private readonly serviceName: string,
+    private readonly errors: ErrorList<ErrorCode>
+  ) {}
 
-    let fullCode = this.service + '/' + code;
-    let message: string;
+  create(code: ErrorCode, data: ErrorData = {}): FirebaseError {
+    const fullCode = `${this.service}/${code}`;
+    const template = this.errors[code];
 
-    if (template === undefined) {
-      message = 'Error';
-    } else {
-      message = template.replace(this.pattern, (match, key) => {
-        let value = data![key];
-        return value !== undefined ? value.toString() : '<' + key + '?>';
-      });
-    }
+    const message = template ? replaceTemplate(template, data) : 'Error';
 
     // Service: Error message (service/code).
-    message = this.serviceName + ': ' + message + ' (' + fullCode + ').';
-    let err = new FirebaseError(fullCode, message);
-
-    // Populate the Error object with message parts for programmatic
-    // accesses (e.g., e.file).
-    for (let prop in data) {
-      if (!data.hasOwnProperty(prop) || prop.slice(-1) === '_') {
-        continue;
+    const fullMessage = `${this.serviceName}: ${message} (${fullCode}).`;
+
+    // Keys with an underscore at the end of their name are not included in
+    // error.data for some reason.
+    const filteredData: ErrorData = {};
+    // TODO: Replace with Object.entries when lib is updated to es2017.
+    for (const key of Object.keys(data)) {
+      if (key.slice(-1) !== '_') {
+        filteredData[key] = data[key];
       }
-      (err as any)[prop] = data[prop];
     }
-
-    return err;
+    return new FirebaseError(fullCode, fullMessage, filteredData);
   }
 }
+
+function replaceTemplate(template: string, data: ErrorData): string {
+  return template.replace(PATTERN, (_, key) => {
+    let value = data != null ? data[key] : undefined;
+    return value != null ? value.toString() : `<${key}?>`;
+  });
+}
+
+const PATTERN = /\{\$([^}]+)}/g;

packages/util/test/errors.test.ts

@@ -15,96 +15,76 @@
  * limitations under the License.
  */
 import { assert } from 'chai';
-import { ErrorFactory, ErrorList, patchCapture } from '../src/errors';
+import { ErrorFactory, ErrorList, FirebaseError } from '../src/errors';
 
 type Err = 'generic-error' | 'file-not-found' | 'anon-replace';
 
-let errors = {
+let errors: ErrorList<Err> = {
   'generic-error': 'Unknown error',
   'file-not-found': "Could not find file: '{$file}'",
   'anon-replace': 'Hello, {$repl_}!'
-} as ErrorList<Err>;
+};
 
 let error = new ErrorFactory<Err>('fake', 'Fake', errors);
 
 describe('FirebaseError', () => {
-  it('create', () => {
+  it('creates an Error', () => {
     let e = error.create('generic-error');
+    assert.instanceOf(e, Error);
+    assert.instanceOf(e, FirebaseError);
     assert.equal(e.code, 'fake/generic-error');
     assert.equal(e.message, 'Fake: Unknown error (fake/generic-error).');
   });
 
-  it('String replacement', () => {
+  it('replaces template values with data', () => {
     let e = error.create('file-not-found', { file: 'foo.txt' });
     assert.equal(e.code, 'fake/file-not-found');
     assert.equal(
       e.message,
       "Fake: Could not find file: 'foo.txt' (fake/file-not-found)."
     );
-    assert.equal((e as any).file, 'foo.txt');
+    assert.equal(e.data.file, 'foo.txt');
   });
 
-  it('Anonymous String replacement', () => {
+  it('anonymously replaces template values with data', () => {
     let e = error.create('anon-replace', { repl_: 'world' });
     assert.equal(e.code, 'fake/anon-replace');
     assert.equal(e.message, 'Fake: Hello, world! (fake/anon-replace).');
-    assert.isUndefined((e as any).repl_);
+    assert.isUndefined(e.data.repl_);
   });
 
-  it('Missing template', () => {
+  it('uses "Error" as template when template is missing', () => {
     // Cast to avoid compile-time error.
     let e = error.create(('no-such-code' as any) as Err);
     assert.equal(e.code, 'fake/no-such-code');
     assert.equal(e.message, 'Fake: Error (fake/no-such-code).');
   });
 
-  it('Missing replacement', () => {
+  it('uses the key in the template if the replacement is missing', () => {
     let e = error.create('file-not-found', { fileX: 'foo.txt' });
     assert.equal(e.code, 'fake/file-not-found');
     assert.equal(
       e.message,
       "Fake: Could not find file: '<file?>' (fake/file-not-found)."
     );
   });
-});
-
-// Run the stack trace tests with, and without, Error.captureStackTrace
-let realCapture = patchCapture();
-stackTests(realCapture);
-stackTests(undefined);
-
-function stackTests(fakeCapture: any) {
-  let saveCapture: any;
-
-  describe(
-    'Error#stack tests - Error.captureStackTrace is ' +
-      (fakeCapture ? 'defined' : 'NOT defined'),
-    () => {
-      before(() => {
-        saveCapture = patchCapture(fakeCapture);
-      });
-
-      after(() => {
-        patchCapture(saveCapture);
-      });
 
-      it('has stack', () => {
-        let e = error.create('generic-error');
-        // Multi-line match trick - .* does not match \n
-        assert.match(e.stack, /FirebaseError[\s\S]/);
-      });
+  it('has stack', () => {
+    let e = error.create('generic-error');
+    // Multi-line match trick - .* does not match \n
+    assert.match(e.stack, /FirebaseError[\s\S]/);
+  });
 
-      it('stack frames', () => {
-        try {
-          dummy1();
-          assert.ok(false);
-        } catch (e) {
-          assert.match(e.stack, /dummy2[\s\S]*?dummy1/);
-        }
-      });
+  it('has function names in stack trace in correct order', () => {
+    try {
+      dummy1();
+      assert.ok(false);
+    } catch (e) {
+      assert.instanceOf(e, FirebaseError);
+      assert.match(e.stack, /dummy2[\s\S]*?dummy1/);
     }
-  );
-}
+  });
+});
 
 function dummy1() {
   dummy2();