Repository: stripe/stripe-node
Pull request #672 (2019-08-02 18:50:28)

Separate super-deprecated errors from base errors, fix type override, and remove name override
This is a follow-on to https://github.com/stripe/stripe-node/pull/661

It makes several small changes but the most consequential are:

1) Adding `get type()` support on `StripeError`, so subclasses have the property `new StripeFooClass({}).type === 'StripeFooClass'`, which is an important property to uphold. We missed this in #661 because our `StripeError` tests had actually been testing `GenericError` on accident, oops!
2) Remove support for magical `.name`, which turned out not to be important for backcompat (previously `.name` was just `'Error'`) – was a mistake for me to ask for that.
3) Have `StripeError` inherit directly from `Error`, and have the default export of `Error.js` be an "old-school class" to maximize backcompat without any complexity leaking into our proper base class, and to emphasize that it is totally deprecated and will be going completely away. Users should not be relying on the stripe library to extend Error.

r? @richardm-stripe
cc @stripe/api-libraries 

remi-stripe: Lurk: I don;t fully understand the changes, but we should make sure we're trying to be aligned across languages, and when we're not that we know why. We're doing something similar to make access errors better in ruby https://github.com/stripe/stripe-ruby/pull/811 and have decided that it's cleaner to do `e.error.code` instead of `e.code`. It might be what you're doing I just wanted to flag.
cc @ob-stripe @brandur-stripe 
rattrayalex-stripe: Ah, thanks! That doesn't actually affect this PR but good to know about. We don't have support for `decline_code` in stripe-node yet, so we should add that. As for `e.error.decline_code` vs `e.decline_code`, IMO we should make that decision on a per-language basis, depending on its idioms (and I'm not sure what the right answer is for Ruby or Node). 
rattrayalex-stripe: My pretty strong hunch, though, looking at this a bit more, is that we should add these properties directly to the error in stripe-node. 
remi-stripe: Fair, that's what I meant! Either we document why they are different (more idiomatic or not) or we are consistent but we should make sure to get aligned since today the 4 dynamic libs have no direct access and we're trying to fix that

Patch:
diff --git a/lib/Error.js b/lib/Error.js
index 86bfda86..5b07c0b1 100644
--- a/lib/Error.js
+++ b/lib/Error.js
@@ -1,82 +1,33 @@
 'use strict';
 
+const utils = require('./utils');
+
 /**
- *  @typedef {{ message?: string, type?: string, code?: number, param?: string, detail?: string, headers?: Record<string, string>, requestId?: string, statusCode?: number }} ErrorParams
- */
-/**
- * Generic Error class to wrap any errors returned by stripe-node
+ * StripeError is the base error from which all other more specific Stripe errors derive.
+ * Specifically for errors returned from Stripe's REST API.
  */
-class GenericError extends Error {
-  /**
-   *
-   * @param  {string} type
-   * @param {string} message
-   */
-  constructor(type, message) {
-    super(message);
-    this.type = type || this.constructor.name;
-    // Saving class name in the property of our custom error as a shortcut.
-    this.name = this.constructor.name;
-
-    // Capturing stack trace, excluding constructor call from it.
-    Error.captureStackTrace(this, this.constructor);
+class StripeError extends Error {
+  constructor(raw = {}) {
+    super(raw.message);
+    // This splat is here for back-compat and should be removed in the next major version.
+    this.populate(...arguments);
   }
 
-  /**
-   *
-   * @param {string} [type] - error type name
-   * @param {string} [message]
-   */
-  populate(type, message) {
-    this.type = type;
-    this.message = message;
+  // Allow `new StripeFooError(raw).type === 'StripeFooError'`
+  get type() {
+    return this.constructor.name;
   }
 
   /**
    * DEPRECATED
-   * Please use ES6 class inheritance instead.
-   * @param {{ type: string, message?: string, [k:string]: any }} options
+   * This will be inlined in the constructor in the future.
    */
-  static extend(options) {
-    class customError extends StripeError {
-      /**
-       * @see {@link https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Function/name#Function_names_in_classes}
-       */
-      // @ts-ignore
-      static get name() {
-        return options.type;
-      }
-    }
-    Object.assign(customError.prototype, options);
-    return customError;
-  }
-}
-
-/**
- * StripeError is the base error from which all other more specific Stripe
- * errors derive.
- * (Specifically for errors returned from Stripe's REST API)
- *
- */
-class StripeError extends GenericError {
-  /**
-   *
-   * @param {ErrorParams} [raw]
-   */
-  constructor(raw = {}) {
-    super(undefined, raw.message);
-    this.populate(raw);
-  }
-  /**
-   *
-   * @param {ErrorParams} raw
-   */
-  // @ts-ignore
   populate(raw) {
-    if (!raw || typeof raw !== 'object' || Object.keys(raw).length < 1) {
+    this.raw = raw;
+    if (!raw || typeof raw !== 'object') {
       return;
     }
-    this.raw = raw;
+
     this.rawType = raw.type;
     this.code = raw.code;
     this.param = raw.param;
@@ -89,8 +40,6 @@ class StripeError extends GenericError {
 
   /**
    * Helper factory which takes raw stripe errors and outputs wrapping instances
-   *
-   * @param {ErrorParams} rawStripeError
    */
   static generate(rawStripeError) {
     switch (rawStripeError.type) {
@@ -108,6 +57,23 @@ class StripeError extends GenericError {
         return new GenericError('Generic', 'Unknown Error');
     }
   }
+
+  /**
+   * DEPRECATED
+   * Please use class inheritance instead.
+   */
+  static extend(options) {
+    const type = options.type;
+    class CustomError extends StripeError {
+      // eslint-disable-next-line class-methods-use-this
+      get type() {
+        return type;
+      }
+    }
+    delete options.type;
+    Object.assign(CustomError.prototype, options);
+    return CustomError;
+  }
 }
 
 // Specific Stripe Error types:
@@ -179,10 +145,26 @@ class StripeIdempotencyError extends StripeError {}
 class StripeInvalidGrantError extends StripeError {}
 
 /**
- * DEPRECATED: Default import from this module is deprecated and
- * will be removed in the next major version
+ * DEPRECATED
+ * This is here for backwards compatibility and will be removed in the next major version.
+ */
+function _Error(raw) {
+  this.populate(...arguments);
+  this.stack = new Error(this.message).stack;
+}
+_Error.prototype = Object.create(Error.prototype);
+_Error.prototype.type = 'GenericError';
+_Error.prototype.populate = function(type, message) {
+  this.type = type;
+  this.message = message;
+};
+_Error.extend = utils.protoExtend;
+
+/**
+ * DEPRECATED.
+ * Do not use the default export; it may be removed or change in a future version.
  */
-module.exports = GenericError;
+module.exports = _Error;
 
 module.exports.StripeError = StripeError;
 module.exports.StripeCardError = StripeCardError;


Test patch:
diff --git a/test/Error.spec.js b/test/Error.spec.js
index e8947e4e..7b70aa16 100644
--- a/test/Error.spec.js
+++ b/test/Error.spec.js
@@ -6,11 +6,42 @@ const Error = require('../lib/Error');
 const expect = require('chai').expect;
 
 describe('Error', () => {
-  it('Populates with type and message params', () => {
-    const e = new Error('FooError', 'Foo happened');
-    expect(e).to.have.property('type', 'FooError');
-    expect(e).to.have.property('message', 'Foo happened');
-    expect(e).to.have.property('stack');
+  describe('Deprecated base export (DEPRECATED)', () => {
+    it('Populates with type and message params (DEPRECATED)', () => {
+      const e = new Error('FooError', 'Foo happened');
+      expect(e).to.have.property('type', 'FooError');
+      expect(e).to.have.property('message', 'Foo happened');
+      expect(e).to.have.property('stack');
+    });
+
+    it('can be extended via .extend method, with a weird signature (DEPRECATED)', () => {
+      const Custom = Error.extend({type: 'MyCustomErrorType'});
+      const err = new Custom('MyOverriddenCustomErrorType', 'byaka');
+      expect(err).to.be.instanceOf(Error);
+      expect(err).to.have.property('type', 'MyOverriddenCustomErrorType');
+      expect(err).to.have.property('message', 'byaka');
+    });
+
+    it('can be extended via .extend method, with `populate` overridden (DEPRECATED)', () => {
+      let populateArgs;
+      const Custom = Error.extend({
+        type: 'MyCardError',
+        populate(...args) {
+          populateArgs = args;
+          this.message = 'overridden message';
+          this.detail = args[0].hello;
+          this.customField = 'hi';
+        },
+      });
+      const err = new Custom({hello: 'ee'}, 'foo', 'bar');
+      expect(populateArgs).to.deep.equal([{hello: 'ee'}, 'foo', 'bar']);
+      expect(err).to.be.instanceOf(Error);
+      expect(err).to.have.property('type', 'MyCardError');
+      expect(err).to.have.property('name', 'Error');
+      expect(err).to.have.property('message', 'overridden message');
+      expect(err).to.have.property('detail', 'ee');
+      expect(err).to.have.property('customField', 'hi');
+    });
   });
 
   describe('StripeError', () => {
@@ -54,48 +85,37 @@ describe('Error', () => {
       expect(e).to.have.property('statusCode', 400);
     });
 
-    it('can be extended via .extend method', () => {
-      const Custom = Error.extend({type: 'MyCustomErrorType'});
+    it('has subclasses which provide `.type` as their name', () => {
+      class Foo extends Error.StripeError {}
+      const err = new Foo({message: 'hi'});
+      expect(err).to.have.property('type', 'Foo');
+    });
+
+    it('can be extended via .extend method (DEPRECATED)', () => {
+      const Custom = Error.StripeError.extend({type: 'MyCustomErrorType'});
       const err = new Custom({message: 'byaka'});
       expect(err).to.be.instanceOf(Error.StripeError);
       expect(err).to.have.property('type', 'MyCustomErrorType');
-      expect(err).to.have.property('name', 'MyCustomErrorType');
       expect(err).to.have.property('message', 'byaka');
     });
 
-    it('can create custom error via `extend` export', () => {
-      const Custom = Error.extend({
+    it('can create custom error via `extend` export (DEPRECATED)', () => {
+      let populateArgs;
+      const Custom = Error.StripeError.extend({
         type: 'MyCardError',
-        populate(raw) {
+        populate(...args) {
+          populateArgs = args;
           this.detail = 'hello';
           this.customField = 'hi';
         },
       });
-      const err = new Custom({
-        message: 'ee',
-      });
+      const err = new Custom({message: 'ee'}, 'wat');
+      expect(populateArgs).to.deep.equal([{message: 'ee'}, 'wat']);
       expect(err).to.be.instanceOf(Error.StripeError);
       expect(err).to.have.property('type', 'MyCardError');
-      expect(err).to.have.property('name', 'MyCardError');
       expect(err).to.have.property('message', 'ee');
       expect(err).to.have.property('detail', 'hello');
       expect(err).to.have.property('customField', 'hi');
     });
-
-    it('ignores invalid constructor parameters for StripeError', () => {
-      const a = new Error.StripeError(false, 'a string');
-      expect(a).to.be.instanceOf(Error.StripeError);
-      expect(a).to.have.property('type', 'StripeError');
-      expect(a).to.have.property('message', '');
-
-      const b = new Error.StripeError('a string');
-      expect(b).to.be.instanceOf(Error.StripeError);
-      expect(b).to.have.property('type', 'StripeError');
-      expect(b).to.have.property('message', '');
-
-      const c = new Error.StripeError({some: 'object'}, {another: 'object'});
-      expect(c).to.be.instanceOf(Error.StripeError);
-      expect(c).to.have.property('type', 'StripeError');
-    });
   });
 });