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

Separate super-deprecated errors from base errors, fix type override, and remove name override #672

Merged
merged 1 commit into from
Aug 2, 2019

Conversation

rattrayalex-stripe
Copy link
Contributor

This is a follow-on to #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 Refactor errors to ES6 classes #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
Copy link
Contributor

remi-stripe commented Aug 2, 2019

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 accessing errors better in ruby stripe/stripe-ruby#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
Copy link
Contributor Author

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
Copy link
Contributor Author

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
Copy link
Contributor

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

Copy link
Contributor

@richardm-stripe richardm-stripe left a comment

Choose a reason for hiding this comment

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

All makes sense to me. Can't think of any Javascripty gotchas.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants