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

Clean up StripeError.generate() #853

Merged
merged 1 commit into from
Mar 27, 2020
Merged

Conversation

rattrayalex-stripe
Copy link
Contributor

r? @richardm-stripe
cc @ob-stripe (especially on doc_url which I am adding here – should we add that to all libs?)
cc @stripe/api-libraries
cc @cah-dunn

This addresses #809 and a few related issues I noticed with our handling of errors and their types.

  1. Fix types for the StripeError.generate() static method, which returns the appropriate subclass based on a type string (and is appropriately typed as such).
  2. Add types for the StripeError constructor.
  3. Pull out the types for Errors into their own definition file.
  4. Make generate available at the top-level, so you can do Stripe.errors.generate({}) which I think is much prettier than Stripe.errors.StripeError.generate({}). Feedback welcome on whether this is a good idea or not.
  5. Add types for StripeInvalidGrantError, which I think we had forgotten to add types for before.
  6. Add support for authentication_error and rate_limit_error in .generate().

One tradeoff I made which I don't feel great about is that the constructor/generate is now typed more loosely than the object itself, so for example if you new StripeError() it will have undefined values for rawType, requestId, and headers, even though all of those are typed as being required fields on StripeError instances.

We could either:

  1. Set these to defaults, such as 'unknown', '', {}.
  2. Change the types to be possibly undefined.
  3. In a future major version, throw an error if these fields are instantiated as undefined.
  4. Shrug – it's useful to have the types for the typical case of when the API is returning things (and we ~know they'll be there), and if developers are mocking things in tests, it's not too surprising that something is undefined even though TS said it wouldn't be. Plus, who looks there?

I'm leaning towards (2) or (3+4) and would value input from the community and from fellow Stripes.

@ob-stripe
Copy link
Contributor

cc @ob-stripe (especially on doc_url which I am adding here – should we add that to all libs?)

Most of them should have it already, and those that don't, should :)

@richardm-stripe
Copy link
Contributor

LGTM. I like option 3, and 4 in the meantime.

@rattrayalex-stripe
Copy link
Contributor Author

Great – I'll wait a bit to see if @cah-dunn has any feedback, and otherwise ship.

@cah-kyle-dunn
Copy link

@rattrayalex-stripe This looks great. Thanks for all of the fixes and QoL changes!

@rattrayalex-stripe rattrayalex-stripe merged commit 67f3f28 into master Mar 27, 2020
@rattrayalex-stripe rattrayalex-stripe deleted the ralex/ts-error-generate branch March 27, 2020 22:31
@rattrayalex-stripe
Copy link
Contributor Author

Thanks Kyle! Thanks for the thorough report!

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.

5 participants