Address comments and fix a few issues

Author: mmermerkaya

packages/util/src/errors.ts

@@ -61,40 +61,33 @@ export type ErrorList<T extends string = string> = {
 const ERROR_NAME = 'FirebaseError';
 
 export interface StringLike {
-  toString: () => string;
+  toString(): string;
 }
 
 export interface ErrorData {
   [key: string]: StringLike | undefined;
 }
 
-export interface FirebaseError {
-  // Unique code for error - format is service/error-code-string
+export interface FirebaseError extends Error, ErrorData {
+  // Unique code for error - format is service/error-code-string.
   readonly code: string;
 
   // Developer-friendly error message.
   readonly message: string;
 
-  // Always 'FirebaseError'
+  // Always 'FirebaseError'.
   readonly name: typeof ERROR_NAME;
 
-  // Where available - stack backtrace in a string
-  readonly stack: string;
-
-  // Additional custom error data that was used in the template.
-  readonly data: ErrorData;
+  // Where available - stack backtrace in a string.
+  readonly stack?: string;
 }
 
 // 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(
-    readonly code: string,
-    message: string,
-    readonly data: ErrorData = {}
-  ) {
+  constructor(readonly code: string, message: string) {
     super(message);
 
     // Fix For ES5
@@ -127,20 +120,25 @@ export class ErrorFactory<ErrorCode extends string> {
 
     // Keys with an underscore at the end of their name are not included in
     // error.data for some reason.
-    const filteredData: ErrorData = {};
+    const error = new FirebaseError(fullCode, fullMessage);
     // 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];
+        if (key in error) {
+          console.warn(
+            `Overwriting FirebaseError base field "${key}" can cause unexpected behavior.`
+          );
+        }
+        error[key] = data[key];
       }
     }
-    return new FirebaseError(fullCode, fullMessage, filteredData);
+    return error;
   }
 }
 
 function replaceTemplate(template: string, data: ErrorData): string {
   return template.replace(PATTERN, (_, key) => {
-    let value = data != null ? data[key] : undefined;
+    const value = data[key];
     return value != null ? value.toString() : `<${key}?>`;
   });
 }

packages/util/test/errors.test.ts

@@ -15,62 +15,84 @@
  * limitations under the License.
  */
 import { assert } from 'chai';
+import { stub } from 'sinon';
 import { ErrorFactory, ErrorList, FirebaseError } from '../src/errors';
 
-type Err = 'generic-error' | 'file-not-found' | 'anon-replace';
+type ErrorCode =
+  | 'generic-error'
+  | 'file-not-found'
+  | 'anon-replace'
+  | 'overwrite-field';
 
-let errors: ErrorList<Err> = {
+const ERROR_MAP: ErrorList<ErrorCode> = {
   'generic-error': 'Unknown error',
   'file-not-found': "Could not find file: '{$file}'",
-  'anon-replace': 'Hello, {$repl_}!'
+  'anon-replace': 'Hello, {$repl_}!',
+  'overwrite-field':
+    'I decided to use {$code} to represent the error code from my server.'
 };
 
-let error = new ErrorFactory<Err>('fake', 'Fake', errors);
+const ERROR_FACTORY = new ErrorFactory<ErrorCode>('fake', 'Fake', ERROR_MAP);
 
 describe('FirebaseError', () => {
   it('creates an Error', () => {
-    let e = error.create('generic-error');
+    const e = ERROR_FACTORY.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('replaces template values with data', () => {
-    let e = error.create('file-not-found', { file: 'foo.txt' });
+    const e = ERROR_FACTORY.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.data.file, 'foo.txt');
+    assert.equal(e.file, 'foo.txt');
   });
 
   it('anonymously replaces template values with data', () => {
-    let e = error.create('anon-replace', { repl_: 'world' });
+    const e = ERROR_FACTORY.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.data.repl_);
+    assert.isUndefined(e.repl_);
   });
 
   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);
+    const e = ERROR_FACTORY.create(('no-such-code' as any) as ErrorCode);
     assert.equal(e.code, 'fake/no-such-code');
     assert.equal(e.message, 'Fake: Error (fake/no-such-code).');
   });
 
   it('uses the key in the template if the replacement is missing', () => {
-    let e = error.create('file-not-found', { fileX: 'foo.txt' });
+    const e = ERROR_FACTORY.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)."
     );
   });
 
+  it('warns if overwriting a base error field with custom data', () => {
+    const warnStub = stub(console, 'warn');
+    const e = ERROR_FACTORY.create('overwrite-field', {
+      code: 'overwritten code'
+    });
+    assert.equal(e.code, 'overwritten code');
+    // TODO: use sinon-chai for this.
+    assert.ok(
+      warnStub.calledOnceWith(
+        'Overwriting FirebaseError base field "code" can cause unexpected behavior.'
+      )
+    );
+    warnStub.restore();
+  });
+
   it('has stack', () => {
-    let e = error.create('generic-error');
+    const e = ERROR_FACTORY.create('generic-error');
     // Multi-line match trick - .* does not match \n
     assert.match(e.stack, /FirebaseError[\s\S]/);
   });
@@ -91,6 +113,6 @@ function dummy1() {
 }
 
 function dummy2() {
-  let error = new ErrorFactory<Err>('dummy', 'Dummy', errors);
+  const error = new ErrorFactory<ErrorCode>('dummy', 'Dummy', ERROR_MAP);
   throw error.create('generic-error');
 }