Refactor errors to ES6 classes

[tinovyatkin]

Your Node.JS requirements at package.json is 6.x as minimum, and it's fully supports ES6 classes. Your are using legacy extending everywhere, so, I think errors classes as smallest and non-breaking change will be good foundation for migration.

Forced to adjust ESLint config, as rule was conflicting with JSDoc comments for constructor and Prettier. And while we was there, removed unused flowtype ESlint plugin and unneeded babel-eslint (ESLint since version 5.x supports all that syntax)

I would love to refactor stripe.js into real class, but it's gonna be a breaking change, so, didn't touch for now.

[rattrayalex-stripe]

Wow, this is awesome; thank you @tinovyatkin !

At first glance looks great.

Going to want to think about whether this could result in any unintended breaking changes for users, or other unintended consequences... (Can you think of anything?)

I'm also curious if we should hold off on this until we're able to change everything to uses proper classes.

Hope to give this a more thorough think/look soon – hopefully today, but may not be until early next week.

[tinovyatkin]

As I wrote initially, I've selected errors classes as a relatively small non-breaking change and where class refactoring in new code will really stands out (imho version with classes looks way more beautiful, and that what is Stripe all about, right? ;-)).
The only official documentation about Node.JS errors is this https://stripe.com/docs/api/errors and these changes breaks nothing from it. I can't image someone relying on deep internals of previous Errors implementation in a way that this will be a breaking change.

Not sure about Stripe policy, but I love small PRs, and refactoring everything to class will be a huge task. You have two candidates left for class refactoring - Stripe from stripe.js (breaking due to documentation) and StripResource. If this will be merged I can do StripeResource refactoring then.

[rattrayalex-stripe]

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

[rattrayalex-stripe]

Thanks, yes, this does make the code more beautiful and we do appreciate that 😄

We do consider some undocumented features to be backwards-breaking, especially if we think there's a reasonable chance that reasonable users may have relied on undocumented behavior. I am hopeful that in this case we'll be okay.

If we don't merge this in now, though, I can certainly create a long-running branch for ES6 classes and merge it into that. We do this fairly frequently to keep PR's small while batching major changes.

[tinovyatkin]

I've also ported errors descriptions from stripe-ruby to JSDoc, so now they have nice IDE description tips:

And renamed the PR to make title less ambiguous and steal from you a chance to wait for complete class refactoring instead of merging this :-).

[rattrayalex-stripe]

This looks really great.

I definitely think some users may have written code like this:

const {StripeError, StripeCardError} = require('stripe/lib/Error')
const MyStripeError = StripeError.extend({type: 'MyError'})
const My StripeCardError = StripeCardError.extend({
  type: 'MyCardError', 
  populate(raw) {
    this.detail = "hello"
    this.customField = "hi"
  }
})

Can we add a shim with tests demonstrating that these work?

[rattrayalex-stripe]

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?

[tinovyatkin]

@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.

[rattrayalex-stripe]

Weird, I think github may be eating some of my comments? But I think we can actually revert most of f3528b4 and just keep this (plus the tests):

/**
 * DEPRECATED 
 * Please use ES6 class inheritance instead.
 * @param {{ type: string, message?: string, [k:string]: any }} options
 */
static extend(options) {
  class DeprecatedCustomError extends GenericError {}
  DeprecatedCustomError.type = options.type;
  for (const property in options) {
    DeprecatedCustomError.prototype[property] = options[property];
  }
  return DeprecatedCustomError;
}

unless I'm missing something?

Sadly, I also think we do need to support this; edge case-y though it may be, it's part of our API:

const Error_ = require('stripe/lib/Error')
Error_.extend({type: 'Hi'})

[tinovyatkin]

Fixed breaking change, and found a nice workaround with static get name class property, so, looks neat now.
Just read in Twitter yesterday "What is your favorite commit message?" and Reversing... was a clear winner 🥇 😄

[rattrayalex-stripe]

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

[rattrayalex-stripe]

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...

[rattrayalex-stripe]

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?

[tinovyatkin]

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...

[rattrayalex-stripe]

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.

[tinovyatkin]

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

[rattrayalex-stripe]

hmm, I think this isn't quite right...
Would have unexpected behavior in cases like these

new StripeError(false, 'a string')
new StripeError('a string')
new StripeError({some: 'object'}, {another: 'object'})

eg; I think this might set type or even throw in places that the old way would not have...
getting super edge-casey here of course.

I think this could maybe be solved by overriding the constructor to be more sane in StripeError, or with another layer of indirection...

[tinovyatkin]

That's really super edge cases, particularly because original implementation doesn't do any typecheck and we can run in the same problems. In Javascript you always can construct an object with some property getter/setter or well known symbol implementation that will throw any function.

But ok. What is the expected behavior in your examples? To throw or to ignore invalid parameters?

[tinovyatkin]

Assuming expected behavior is like a standard errors (never throw, use as much information from params as possible) I've added typechecks and edge cases tests.
It doesn't make code any more beautiful, but bulletproof!

[rattrayalex-stripe]

@tinovyatkin I really hate to be difficult here, but I'm uncomfortable having our base error class contain that sort of tangled logic for a deprecated, legacy edge-case. I'd really like to disentangle the two. My suggestion here still feels much better than this, though you're welcome to implement the same idea with classes if you like?

All that said, I'm impressed and grateful for all your hard work here, and don't want to impose further on your time – I know I've asked a lot here. I'm happy to merge this as-is and make that change myself if you like? Up to you!

[tinovyatkin]

@rattrayalex-stripe I really don't want to have mixed in the same file use of class and function.prototype that's why I don't want to implement your suggestion.

My last attempt here is to return constructor to StripeError (it will be needed anyway if one day you will decide to migrate this to TypeScript) and make behavior of edge cases close to original implementation (that was trowing, actually, on some of these calls).

[rattrayalex-stripe]

Okay, totally fair 😄you've done a ton here already it's much appreciated. I think it'd be pretty unfair for me to ask even more of you, and I'm excited to see the bulk of this land!

LGTM

I'm going to want to make some adjustments before releasing this, so I'll hold off on merging until my own work is ready.