Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Refactor errors to ES6 classes #661

Merged
merged 14 commits into from
Aug 2, 2019
Merged
5 changes: 3 additions & 2 deletions .eslintrc.js
Original file line number Diff line number Diff line change
Expand Up @@ -62,7 +62,6 @@ module.exports = {
],
'line-comment-position': 'off',
'linebreak-style': ['error', 'unix'],
'lines-around-comment': 'error',
'lines-around-directive': 'error',
'max-depth': 'error',
'max-len': 'off',
Expand Down Expand Up @@ -245,7 +244,9 @@ module.exports = {
'yield-star-spacing': 'error',
yoda: ['error', 'never'],
},
parser: 'babel-eslint',
parserOptions: {
ecmaVersion: 2018,
},
plugins: ['prettier'],
extends: ['plugin:prettier/recommended'],
};
226 changes: 153 additions & 73 deletions lib/Error.js
Original file line number Diff line number Diff line change
@@ -1,94 +1,174 @@
'use strict';

const utils = require('./utils');

module.exports = _Error;

/**
* Generic Error klass to wrap any errors returned by stripe-node
* Generic Error class to wrap any errors returned by stripe-node
*/
function _Error(raw) {
this.populate(...arguments);
this.stack = new Error(this.message).stack;
}

// Extend Native Error
_Error.prototype = Object.create(Error.prototype);
class GenericError extends Error {
constructor(message) {
super(message);
// Saving class name in the property of our custom error as a shortcut.
this.type = this.constructor.name;
this.name = this.constructor.name;

_Error.prototype.type = 'GenericError';
_Error.prototype.populate = function(type, message) {
this.type = type;
this.message = message;
};

_Error.extend = utils.protoExtend;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is the function most likely to have been used by folks outside of Stripe, if for some reason they decided to define their own errors as an extend of some StripeError. Is there a way to preserve it?

Copy link
Contributor Author

@tinovyatkin tinovyatkin Jul 30, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@rattrayalex-stripe did as much as possible (the only case that will be breaking now is default export that is not a function itself). We can do even that, but it will look really weird and for very edge use case.

// Capturing stack trace, excluding constructor call from it.
Error.captureStackTrace(this, this.constructor);
}

/**
* Create subclass of internal Error klass
* (Specifically for errors returned from Stripe's REST API)
*/
const StripeError = (_Error.StripeError = _Error.extend({
type: 'StripeError',
populate(raw) {
// Move from prototype def (so it appears in stringified obj)
this.type = this.type;

this.stack = new Error(raw.message).stack;
if (!raw || typeof raw !== 'object' || Object.keys(raw).length < 1) {
return;
}
this.raw = raw;
this.rawType = raw.type;
this.code = raw.code;
this.param = raw.param;
this.message = raw.message;
this.detail = raw.detail;
this.raw = raw;
this.headers = raw.headers;
this.requestId = raw.requestId;
this.statusCode = raw.statusCode;
},
}));
}

/**
* DEPRECATED
* Please use ES6 class inheritance instead.
* @param {{ type: string, message?: string, [k:string]: any }} options
*/
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;
}
}

/**
* Helper factory which takes raw stripe errors and outputs wrapping instances
* StripeError is the base error from which all other more specific Stripe
* errors derive.
* (Specifically for errors returned from Stripe's REST API)
*
* @typedef {{ message?: string, type?: string, code?: number, param?: string, detail?: string, headers?: Record<string, string>, requestId?: string, statusCode?: number }} ErrorParams
*/
StripeError.generate = (rawStripeError) => {
switch (rawStripeError.type) {
case 'card_error':
return new _Error.StripeCardError(rawStripeError);
case 'invalid_request_error':
return new _Error.StripeInvalidRequestError(rawStripeError);
case 'api_error':
return new _Error.StripeAPIError(rawStripeError);
case 'idempotency_error':
return new _Error.StripeIdempotencyError(rawStripeError);
case 'invalid_grant':
return new _Error.StripeInvalidGrantError(rawStripeError);
class StripeError extends GenericError {
/**
*
* @param {ErrorParams} raw
*/
constructor(raw) {
super(raw.message);
this.populate(raw);
}
return new _Error('Generic', 'Unknown Error');
};

/**
* Helper factory which takes raw stripe errors and outputs wrapping instances
*
* @param {ErrorParams} rawStripeError
*/
static generate(rawStripeError) {
switch (rawStripeError.type) {
case 'card_error':
return new StripeCardError(rawStripeError);
case 'invalid_request_error':
return new StripeInvalidRequestError(rawStripeError);
case 'api_error':
return new StripeAPIError(rawStripeError);
case 'idempotency_error':
return new StripeIdempotencyError(rawStripeError);
case 'invalid_grant':
return new StripeInvalidGrantError(rawStripeError);
default:
return new GenericError('Unknown Error');
}
}
}

// Specific Stripe Error types:
_Error.StripeCardError = StripeError.extend({type: 'StripeCardError'});
_Error.StripeInvalidRequestError = StripeError.extend({
type: 'StripeInvalidRequestError',
});
_Error.StripeAPIError = StripeError.extend({type: 'StripeAPIError'});
_Error.StripeAuthenticationError = StripeError.extend({
type: 'StripeAuthenticationError',
});
_Error.StripePermissionError = StripeError.extend({
type: 'StripePermissionError',
});
_Error.StripeRateLimitError = StripeError.extend({
type: 'StripeRateLimitError',
});
_Error.StripeConnectionError = StripeError.extend({
type: 'StripeConnectionError',
});
_Error.StripeSignatureVerificationError = StripeError.extend({
type: 'StripeSignatureVerificationError',
});
_Error.StripeIdempotencyError = StripeError.extend({
type: 'StripeIdempotencyError',
});
_Error.StripeInvalidGrantError = StripeError.extend({
type: 'StripeInvalidGrantError',
});

/**
* CardError is raised when a user enters a card that can't be charged for
* some reason.
*/
class StripeCardError extends StripeError {}

/**
* InvalidRequestError is raised when a request is initiated with invalid
* parameters.
*/
class StripeInvalidRequestError extends StripeError {}

/**
* APIError is a generic error that may be raised in cases where none of the
* other named errors cover the problem. It could also be raised in the case
* that a new error has been introduced in the API, but this version of the
* Node.JS SDK doesn't know how to handle it.
*/
class StripeAPIError extends StripeError {}

/**
* AuthenticationError is raised when invalid credentials are used to connect
* to Stripe's servers.
*/
class StripeAuthenticationError extends StripeError {}

/**
* PermissionError is raised in cases where access was attempted on a resource
* that wasn't allowed.
*/
class StripePermissionError extends StripeError {}

/**
* RateLimitError is raised in cases where an account is putting too much load
* on Stripe's API servers (usually by performing too many requests). Please
* back off on request rate.
*/
class StripeRateLimitError extends StripeError {}

/**
* StripeConnectionError is raised in the event that the SDK can't connect to
* Stripe's servers. That can be for a variety of different reasons from a
* downed network to a bad TLS certificate.
*/
class StripeConnectionError extends StripeError {}

/**
* SignatureVerificationError is raised when the signature verification for a
* webhook fails
*/
class StripeSignatureVerificationError extends StripeError {}

/**
* IdempotencyError is raised in cases where an idempotency key was used
* improperly.
*/
class StripeIdempotencyError extends StripeError {}

/**
* InvalidGrantError is raised when a specified code doesn't exist, is
* expired, has been used, or doesn't belong to you; a refresh token doesn't
* exist, or doesn't belong to you; or if an API key's mode (live or test)
* doesn't match the mode of a code or refresh token.
*/
class StripeInvalidGrantError extends StripeError {}

module.exports = {
GenericError,
StripeError,
StripeCardError,
StripeInvalidRequestError,
StripeAPIError,
StripeAuthenticationError,
StripePermissionError,
StripeRateLimitError,
StripeConnectionError,
StripeSignatureVerificationError,
StripeIdempotencyError,
StripeInvalidGrantError,
extend: GenericError.extend.bind(GenericError),
};
2 changes: 1 addition & 1 deletion lib/StripeResource.js
Original file line number Diff line number Diff line change
Expand Up @@ -88,7 +88,7 @@ StripeResource.prototype = {

_timeoutHandler(timeout, req, callback) {
return () => {
const timeoutErr = new Error('ETIMEDOUT');
const timeoutErr = new TypeError('ETIMEDOUT');
timeoutErr.code = 'ETIMEDOUT';

req._isAborted = true;
Expand Down
12 changes: 3 additions & 9 deletions lib/resources/Files.js
Original file line number Diff line number Diff line change
Expand Up @@ -3,10 +3,12 @@
const Buffer = require('safe-buffer').Buffer;
const utils = require('../utils');
const multipartDataGenerator = require('../MultipartDataGenerator');
const Error = require('../Error');
const {StripeError} = require('../Error');
const StripeResource = require('../StripeResource');
const stripeMethod = StripeResource.method;

class StreamProcessingError extends StripeError {}

module.exports = StripeResource.extend({
path: 'files',

Expand Down Expand Up @@ -50,14 +52,6 @@ module.exports = StripeResource.extend({
}

function streamError(callback) {
const StreamProcessingError = Error.extend({
type: 'StreamProcessingError',
populate(raw) {
this.type = this.type;
this.message = raw.message;
this.detail = raw.detail;
},
});
return (error) => {
callback(
new StreamProcessingError({
Expand Down
2 changes: 0 additions & 2 deletions package.json
Original file line number Diff line number Diff line change
Expand Up @@ -26,14 +26,12 @@
},
"main": "lib/stripe.js",
"devDependencies": {
"babel-eslint": "^10.0.1",
"chai": "~4.2.0",
"chai-as-promised": "~7.1.1",
"coveralls": "^3.0.0",
"eslint": "^5.16.0",
"eslint-config-prettier": "^4.1.0",
"eslint-plugin-chai-friendly": "^0.4.0",
"eslint-plugin-flowtype": "^3.8.1",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

good catch, not sure how that got in there...

"eslint-plugin-prettier": "^3.0.1",
"mocha": "~6.1.4",
"nock": "^10.0.6",
Expand Down
32 changes: 30 additions & 2 deletions test/Error.spec.js
Original file line number Diff line number Diff line change
Expand Up @@ -7,8 +7,8 @@ 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');
const e = new Error.GenericError('Foo happened');
expect(e).to.have.property('type', 'GenericError');
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can this test change be reverted? I think this test should continue to pass.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ahhh, I see, this is what you were talking about. The Error exported from the root has a totally different signature from GenericError, and also doesn't make much sense to extend... hmm...

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should probably keep all the code for function _Error(raw) { and just mark it as deprecated (and not actually inherit from it in the ES6 classes). Thoughts?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Error here is default import from Error.js. To make it pass we should be doing weird things as I commented earlier, like this:

module.exports = GenericError;
module.exports.StripeError = StripeError;

I mean, export GenericError with other errors as attached properties on it...
The example you put makes sense, extend export do work, but doing this will look waoh...

Copy link
Contributor

@rattrayalex-stripe rattrayalex-stripe Jul 31, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm thinking this:

module.exports = _Error;
module.exports.StripeError = class StripeError {...}
module.exports.StripeFooError = class StripeFooError extends StripeError;

/**
 * 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;
...

Yes, the legacy code is ugly; let's just shunt it to the bottom, mark it as deprecated, and not have the rest of our classes build on it.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Weeeell... Can't do THAT ugly. Passing all original tests now ✅

expect(e).to.have.property('message', 'Foo happened');
expect(e).to.have.property('stack');
});
Expand Down Expand Up @@ -53,5 +53,33 @@ describe('Error', () => {
});
expect(e).to.have.property('statusCode', 400);
});

it('can be extended via .extend method', () => {
const Custom = Error.GenericError.extend({type: 'MyCustomErrorType'});
const err = new Custom({message: 'byaka'});
expect(err).to.be.instanceOf(Error.GenericError);
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({
type: 'MyCardError',
populate(raw) {
this.detail = 'hello';
this.customField = 'hi';
},
});
const err = new Custom({
message: 'ee',
});
expect(err).to.be.instanceOf(Error.GenericError);
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');
});
});
});
Loading