Update error types to be namespaced under Stripe.error #1375
Conversation
There was a problem hiding this comment.
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.
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.errorsand are not accessible at the top-level (eg.Stripe.StripeErroris 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?
pakrym-stripe
changed the title
|
Done, duplicated types so instanceof still compiles added deprecated description to everything. |
There was a problem hiding this comment.
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.
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.
remi-stripe
deleted the
pakrym/Move_error_type_definitions_into_a_namespace
branch
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: