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

Invoice.id is shouldn't be optional #1500

Closed
deldrid1 opened this issue Aug 3, 2022 · 7 comments · Fixed by #1629
Closed

Invoice.id is shouldn't be optional #1500

deldrid1 opened this issue Aug 3, 2022 · 7 comments · Fixed by #1629
Assignees
Labels

Comments

@deldrid1
Copy link

deldrid1 commented Aug 3, 2022

Describe the bug

According to https://stripe.com/docs/api/invoices/line_item#invoice_line_item_object-id, invoice.id should be required.

As I am upgrading to v10.0.0, it looks like it was made optional

To Reproduce

Perform any operation on an invoice and get an incorrectly typed object as the response.

Expected behavior

This should be a required property so I don't have to litter my code with unnecessary run-time undefined checks.

Code snippets

No response

OS

macOS

Node version

All Node Version

Library version

stripe-node v10.0.0

API version

2022-08-01

Additional context

No response

@deldrid1 deldrid1 added the bug label Aug 3, 2022
@kamil-stripe kamil-stripe self-assigned this Aug 4, 2022
@snlamm
Copy link

snlamm commented Sep 8, 2022

Any update on this? Would like to avoid all the extra null checks 🙂

@kamil-stripe
Copy link
Contributor

We're working on the issue with the product team. Apologies for the delay. The resource represents both Invoice and UpcomingInvoice, where the ID is optional. That's why it was made optional and the fix is not as easy as reverting the change.

@snlamm
Copy link

snlamm commented Sep 8, 2022

Thanks for the update!

@michelbl
Copy link

Any update on this issue ? The type is still optional for v11.1.0 : https://github.com/stripe/stripe-node/blob/v11.1.0/types/2022-11-15/Invoices.d.ts#L43

@julestruong
Copy link

Can someone from stripe explain why it was done like this ? and not with a upcomingInvoice Type ? as done in the PR ?

@richardm-stripe
Copy link
Contributor

Hello @julestruong.

Our default strategy is to establish a single type for each "resource" that is broad enough to capture the return values of all the methods on that resource, so because retrieveUpcoming is on the Invoice resource, we expanded the Invoice type to be able to describe its return value and made id optional.

We have a pretty strong bias against making exceptions to our default patterns. Even when an exception makes an interface locally better, a library that's full of special cases is globally worse. Given all the feedback here though, it's pretty clear that #1629 is the way to go.

@julestruong
Copy link

That was really nice of you to take the time to explain your strategy.
Thank you.
I'll patch the changes in the PR then , waiting for its merge.

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 a pull request may close this issue.

6 participants