-
Notifications
You must be signed in to change notification settings - Fork 158
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
fix: Stripe types and checks #264
Conversation
What should cd476cd accomplish? |
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.
My main thought is that the checks added in this PR aren't needed if the init:stripe
task is run correctly. So having product.default_price
as Stripe.Price
is acceptable. If product.default_price
is not of type Stripe.Price
, we may have to investigate the init:stripe
task, as it's not working correctly.
Either way, nice work fixing the Stripe types! I've added a suggestion to improve this situation for the whole codebase.
utils/payments.ts
Outdated
@@ -1,4 +1,7 @@ | |||
// Copyright 2023 the Deno authors. All rights reserved. MIT license. | |||
|
|||
// Default types for Stripe don't yet work: https://github.com/stripe-samples/stripe-node-deno-samples/issues/2 | |||
// @deno-types="npm: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.
Can you move this to a stripe.ts
file at the root of this codebase and point the stripe
import in the import_map.json
to that file? That way, all files that import stripe
also get types.
Also, can you please add a version that ensures the types and runtime versions of stripe
match?
I see, in that case a better solution would've been for me to not make these checks but throw a custom error/banner instead of relying on type assertions. That means I could remove most of the code checks. |
Yes, I'd rather the code threw 👍🏾 |
cd476cd
to
ace8fd9
Compare
I'm made the changes. Remove the type assertions and threw and error (had to create a type guard for this to work). I still get an error on the pricing page, even after running |
ace8fd9
to
975ceea
Compare
I've also moved the stripe definitions into |
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.
I made a few fixes, and now LGTM! Thanks for this!
Note for future: please use deno task ok
before git push
-ing to ensure all checks pass.
Fixes #261
Adding types
I started by adding
just above the
Stripe
import.This is based on https://deno.com/manual@v1.34.0/advanced/typescript/types#providing-types-when-importing
This is not the best solution, but currently there's an issue with having types for Stripe in Deno:
stripe-samples/stripe-node-deno-samples#2
Covering all possibilities
After the initial error was fixed, I was receiving a different error, also caused by type assertions, so I had to change
and handle all cases
Testing
git clone ...
deno task start
PS
defaultPrice.recurring?.interval
already has a defined type, but I wasn't sure what's the correct way to import it in Deno. I left it as string since it's not a critical part of the app, it's used just for displaying text. So it's not a high-concern at the moment.