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

Export errors to allow typed access to them #459

Merged
merged 1 commit into from
May 14, 2018

Conversation

brandur
Copy link
Contributor

@brandur brandur commented May 12, 2018

Exports the errors module and the types it contains as stripe.errors.
This allows users to match on an error with the instanceof operator
instead of having to match on the string generated by accessing an
object's type.

There's a bit of an unfortunate singular/plural mismatch (the file is called
Error.js), but I called the export stripe.errors to match the
already-plural stripe.webhooks.

Fixes #457.

r? @jlomas-stripe Hey! Could I get your opinion on this?
cc @stripe/api-libraries

@brandur-stripe
Copy link
Contributor

(And oops — I meant @jlomas-stripe. Sorry @JLomas!)

Exports the errors module and the types it contains as `stripe.errors`.
This allows users to match on an error with the `instanceof` operator
instead of having to match on the string generated by accessing an
object's `type`.

Fixes #457.
@jlomas-stripe
Copy link
Contributor

@brandur-stripe This looks great. It looks like Error.js is only referenced in 3 places:

lib/StripeResource.js
lib/Webhooks.js
test/Error.spec.js

...so maybe we should also take this opportunity rename the file to Errors.js as well? Though ... it should still export Error, so maybe not. ¯\_(ツ)_/¯

Either way,

lgtm

@brandur-stripe
Copy link
Contributor

...so maybe we should also take this opportunity rename the file to Errors.js as well? Though ... it should still export Error, so maybe not. ¯_(ツ)_/¯

Yeah, that's basically what stopped me. I think this would still be a good cleanup, but we should also rename the symbol Error to Errors too, and that would've added quite a bit of noise to this PR.

I'll merge this and then see if that tweak works the way we hope on a different PR. Thanks for taking a look!

@brandur-stripe brandur-stripe merged commit edea141 into master May 14, 2018
@brandur-stripe brandur-stripe deleted the brandur-typed-error-access branch May 14, 2018 16:22
@brandur-stripe
Copy link
Contributor

Alright, I tried to follow this up with a rename, but it turned into quite a deep rabbit hole that I wasn't prepared for. Error.js seems to be doing something to manipulate the base Error that I don't understand, and a simple rename will cause a failure on this check:

  it('Populates with type and message params', function() {
    var e = new Error('FooError', 'Foo happened');
    expect(e).to.have.property('type', 'FooError');

It also has the additional problem that the author conflated a class with a package, and throughout much of the file, the singular _Error is more conceptually correct.

I'm just going to leave it for now.

@SimonSchick
Copy link

Little bit late here but, why are the errors exported on the client rather than directly from the package, this seems a little counter intuitive since errors are not bound to a stripe instance.

@jlomas-stripe
Copy link
Contributor

@SimonSchick What do you mean? What would you expect to be able to do here? Like, const StripeErrors = require('stripe/lib/Error'); works just fine, but I'm not sure you'd ever do that since they're specific to Stripe...?

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

Successfully merging this pull request may close these issues.

4 participants