-
Notifications
You must be signed in to change notification settings - Fork 743
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
Update error types to be namespaced under Stripe.error #1375
Update error types to be namespaced under Stripe.error #1375
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you add some more detail to the pull request description?
Will this break type references to Stripe.Errors.*
? If so, I think the blast radius of this is too big to release outside a major version, and maybe we should consider copying either the runtime type or the Typescript type to both places? Does that make sense?
I updated the PR so old types are still accessible but are marked as deprecated and don't work as a parameter to instanceof call. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks good Pavel!
For the sake of communication, I would:
- Change PR title to be something like "Update error types to be namespaced under Stripe.error"
- Update the PR description to make it clear that this is not a breaking change, and describing the current underlying implementation. Could be something like
This fixes a mismatch between the types and the code. All errors classes are exported through
Stripe.errors
and are not accessible at the top-level (eg.Stripe.StripeError
is not valid - it is exported asStripe.errors.StripeError
). The types however showed these classes at the top-level. This PR brings the types in line with the underlying code while still offering the top-level types with a deprecation notice to avoid breaking existing Typescript implementations.
- On next release, let's make sure to include a similar sentence directly in the CHANGELOG so that users are aware of this.
- Maybe we could add a comment in the types explaining that the deprecated type doesn't actually exist + why we're keeping it?
--
re. the instanceof support - I wonder if we want to keep that but also in a deprecated state? It would be nice to not break existing compilations for either reason. In an ideal world we could just re-export everything at the top-level to make the JS match the types. I also wonder if we used to do that and then something accidentally changed in the JS?
Done, duplicated types so instanceof still compiles added deprecated description to everything. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some small tweaks and then I think we're good to go! We should also do a pass internally on documentation to make sure we're suggesting Stripe.errors.*
everywhere.
PTAL @pakrym-stripe ?
PTAL @dcr-stripe ? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM! Thanks for your work on this Pavel!
Just one thing - let's remove:
Also makes it so *** instanceof Stripe.StripeError; doesn't compile anymore but it seems OK because it never actually worked at runtime.
From the PR description, since now it does compile.
Fixes: #1374
This fixes a mismatch between the types and the code. All errors classes are exported through Stripe.errors and are not accessible at the top-level (eg. Stripe.StripeError is not valid - it is exported as Stripe.errors.StripeError). The types however showed these classes at the top-level. This PR brings the types in line with the underlying code while still offering the top-level types with a deprecation notice to avoid breaking existing Typescript implementations.
Looks like this in the IDE: