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.

Sign up for GitHub

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

Refactor errors to ES6 classes #661

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
14 commits
Select commit Hold shift + click to select a range
82be852
adjust ESLint config removing unused/unneeded plugins and fixing Pret…
tinovyatkin Jul 25, 2019
784caec
refactore errors to be ES6 classes
tinovyatkin Jul 25, 2019
2c5a837
porting errors descriptions from stripe-ruby
tinovyatkin Jul 26, 2019
126447c
remove conflicting rule
tinovyatkin Jul 30, 2019
f3528b4
shim private methods
tinovyatkin Jul 30, 2019
2ae978f
fix GenericError at `generate`
tinovyatkin Jul 30, 2019
2bbe046
reverted GenericError constructor
tinovyatkin Jul 30, 2019
7ffa99d
it's `Object.assign`!
tinovyatkin Jul 30, 2019
4261fc9
add @rattrayalex-stripe deprecation comment
tinovyatkin Jul 30, 2019
62d2748
restore original test
tinovyatkin Jul 31, 2019
ca4e1a4
make GenericError default export
tinovyatkin Jul 31, 2019
841587a
and GenericError at `generate`
tinovyatkin Jul 31, 2019
42e2e8a
handling edge cases as standard Errors
tinovyatkin Jul 31, 2019
1b51929
my last attempt here
tinovyatkin Jul 31, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 4 additions & 2 deletions .eslintrc.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -62,7 +62,7 @@ module.exports = {
],
'line-comment-position': 'off',
'linebreak-style': ['error', 'unix'],
'lines-around-comment': 'error',
'lines-around-comment': ['error', { "allowClassStart": true }],
tinovyatkin marked this conversation as resolved.
Show resolved Hide resolved
'lines-around-directive': 'error',
'max-depth': 'error',
'max-len': 'off',
Expand Down Expand Up @@ -245,7 +245,9 @@ module.exports = {
'yield-star-spacing': 'error',
yoda: ['error', 'never'],
},
parser: 'babel-eslint',
parserOptions: {
ecmaVersion: 2018,
},
plugins: ['prettier'],
extends: ['plugin:prettier/recommended'],
};
149 changes: 77 additions & 72 deletions lib/Error.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,39 +1,32 @@
'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 •
edited
Loading

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
* Create subclass of internal Error class
* (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
*/
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;
class StripeError extends GenericError {
/**
*
* @param {ErrorParams} raw
*/
constructor(raw) {
super(raw.message);
this.rawType = raw.type;
this.code = raw.code;
this.param = raw.param;
Expand All @@ -43,52 +36,64 @@ const StripeError = (_Error.StripeError = _Error.extend({
this.headers = raw.headers;
this.requestId = raw.requestId;
this.statusCode = raw.statusCode;
},
}));
}

/**
* Helper factory which takes raw stripe errors and outputs wrapping instances
*/
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);
/**
* 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');
}
}
return new _Error('Generic', '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',
});

class StripeCardError extends StripeError {}

class StripeInvalidRequestError extends StripeError {}

class StripeAPIError extends StripeError {}

class StripeAuthenticationError extends StripeError {}

class StripePermissionError extends StripeError {}

class StripeRateLimitError extends StripeError {}

class StripeConnectionError extends StripeError {}

class StripeSignatureVerificationError extends StripeError {}

class StripeIdempotencyError extends StripeError {}

class StripeInvalidGrantError extends StripeError {}

module.exports = {
GenericError,
StripeError,
StripeCardError,
StripeInvalidRequestError,
StripeAPIError,
StripeAuthenticationError,
StripePermissionError,
StripeRateLimitError,
StripeConnectionError,
StripeSignatureVerificationError,
StripeIdempotencyError,
StripeInvalidGrantError,
};
2 changes: 1 addition & 1 deletion lib/StripeResource.js
View file
Open in desktop
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
View file
Open in desktop
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
View file
Open in desktop
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
4 changes: 2 additions & 2 deletions test/Error.spec.js
View file
Open in desktop
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 •
edited
Loading

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
31 changes: 2 additions & 29 deletions yarn.lock
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -52,7 +52,7 @@
esutils "^2.0.2"
js-tokens "^4.0.0"

"@babel/parser@^7.0.0", "@babel/parser@^7.4.3", "@babel/parser@^7.4.4":
"@babel/parser@^7.4.3", "@babel/parser@^7.4.4":
version "7.4.4"
resolved "https://registry.yarnpkg.com/@babel/parser/-/parser-7.4.4.tgz#5977129431b8fe33471730d255ce8654ae1250b6"
integrity sha512-5pCS4mOsL+ANsFZGdvNLybx4wtqAZJ0MJjMHxvzI3bvIsz6sQvzW8XX92EYIkiPtIvcfG3Aj+Ir5VNyjnZhP7w==
Expand All @@ -66,7 +66,7 @@
"@babel/parser" "^7.4.4"
"@babel/types" "^7.4.4"

"@babel/traverse@^7.0.0", "@babel/traverse@^7.4.3":
"@babel/traverse@^7.4.3":
version "7.4.4"
resolved "https://registry.yarnpkg.com/@babel/traverse/-/traverse-7.4.4.tgz#0776f038f6d78361860b6823887d4f3937133fe8"
integrity sha512-Gw6qqkw/e6AGzlyj9KnkabJX7VcubqPtkUQVAwkc0wUMldr3A/hezNB3Rc5eIvId95iSGkGIOe5hh1kMKf951A==
Expand Down Expand Up @@ -198,18 +198,6 @@ aws4@^1.8.0:
resolved "https://registry.yarnpkg.com/aws4/-/aws4-1.8.0.tgz#f0e003d9ca9e7f59c7a508945d7b2ef9a04a542f"
integrity sha512-ReZxvNHIOv88FlT7rxcXIIC0fPt4KZqZbOlivyWtXLt8ESx84zd3kMC6iK5jVeS2qt+g7ftS7ye4fi06X5rtRQ==

babel-eslint@^10.0.1:
version "10.0.1"
resolved "https://registry.yarnpkg.com/babel-eslint/-/babel-eslint-10.0.1.tgz#919681dc099614cd7d31d45c8908695092a1faed"
integrity sha512-z7OT1iNV+TjOwHNLLyJk+HN+YVWX+CLE6fPD2SymJZOZQBs+QIexFjhm4keGTm8MW9xr4EC9Q0PbaLB24V5GoQ==
dependencies:
"@babel/code-frame" "^7.0.0"
"@babel/parser" "^7.0.0"
"@babel/traverse" "^7.0.0"
"@babel/types" "^7.0.0"
eslint-scope "3.7.1"
eslint-visitor-keys "^1.0.0"

balanced-match@^1.0.0:
version "1.0.0"
resolved "https://registry.yarnpkg.com/balanced-match/-/balanced-match-1.0.0.tgz#89b4d199ab2bee49de164ea02b89ce462d71b767"
Expand Down Expand Up @@ -556,28 +544,13 @@ eslint-plugin-chai-friendly@^0.4.0:
resolved "https://registry.yarnpkg.com/eslint-plugin-chai-friendly/-/eslint-plugin-chai-friendly-0.4.1.tgz#9eeb17f92277ba80bb64f0e946c6936a3ae707b4"
integrity sha512-hkpLN7VVoGGsofZjUhcQ+sufC3FgqMJwD0DvAcRfxY1tVRyQyVsqpaKnToPHJQOrRo0FQ0fSEDwW2gr4rsNdGA==

eslint-plugin-flowtype@^3.8.1:
version "3.8.1"
resolved "https://registry.yarnpkg.com/eslint-plugin-flowtype/-/eslint-plugin-flowtype-3.8.1.tgz#fefbc160b8f55c0bc0201236471637ad47ca6886"
integrity sha512-7ye/CCdTzytOu54Y1dUed/ejcIPMe0eSlxhfPIhOv2DCMvLMbwYGagEYfEuJ1ylMhkt8Z+rt8VtJXniNwC3hRQ==
dependencies:
lodash "^4.17.11"

eslint-plugin-prettier@^3.0.1:
version "3.0.1"
resolved "https://registry.yarnpkg.com/eslint-plugin-prettier/-/eslint-plugin-prettier-3.0.1.tgz#19d521e3981f69dd6d14f64aec8c6a6ac6eb0b0d"
integrity sha512-/PMttrarPAY78PLvV3xfWibMOdMDl57hmlQ2XqFeA37wd+CJ7WSxV7txqjVPHi/AAFKd2lX0ZqfsOc/i5yFCSQ==
dependencies:
prettier-linter-helpers "^1.0.0"

[email protected]:
version "3.7.1"
resolved "https://registry.yarnpkg.com/eslint-scope/-/eslint-scope-3.7.1.tgz#3d63c3edfda02e06e01a452ad88caacc7cdcb6e8"
integrity sha1-PWPD7f2gLgbgGkUq2IyqzHzctug=
dependencies:
esrecurse "^4.1.0"
estraverse "^4.1.1"

eslint-scope@^4.0.3:
version "4.0.3"
resolved "https://registry.yarnpkg.com/eslint-scope/-/eslint-scope-4.0.3.tgz#ca03833310f6889a3264781aa82e63eb9cfe7848"
Expand Down