TypeScript types are invalid when expanding responses

[isaachinman]

trafficstars

Describe the bug

When expanding responses as per the documentation here, the return types for various Stripe methods are not updated to accurately reflect the modified responses.

To Reproduce

const stripeCustomer = await stripe.customers.retrieve(
  stripeCustomerId,
  {
    expand: ['subscriptions'],
  },
)

Expected behavior

The returned data does indeed contain a nested subscriptions object:

"subscriptions": {
  "object": "list",
  "data": [...]
}

However, the TypeScript types do not declare this at all.

Code snippets

No response

OS

macOS

Node version

Node v16.4.2

Library version

stripe-node v10.8.0

API version

2022-08-01

Additional context

No response

[kamil-stripe]

Hi @isaachinman! Do you mean the subscriptions property defined here should be non-nullable when subscriptions are passed in the expand property?

[isaachinman]

Hey @kamil-stripe, thanks for taking the time to respond.

Although the typing generally seems good for stripe-node, there appears to be significant issues with the subscriptions.list method, and possibly others.

Yes, the subscriptions property should be non-nullable when the expand array contains "subscriptions" – this should be easily achievable with generics.

However, I think there are more basic problems with the typing of this method:

const customer = await stripeClient.customers.retrieve('stripeId')

// Property 'created' does not exist on type 'Response<Customer | DeletedCustomer>'
console.log(customer.created)

[kamil-stripe]

I see. That sounds beneficial to our SDK and users. Do you have an example how would you solve this problem with generics?

Regarding # 2, could you provide a sample project with that problem? I cannot reproduce it. For lines like:

import Stripe from "stripe";
const stripe = new Stripe(/* ... */);

const subscriptions = await stripe.subscriptions.retrieve("sub_xxx");
console.log(subscriptions.created);

and tsconfig like this

{  
    "compilerOptions": {    
        "target": "ES2022",
        "module": "ES2022",
        "moduleResolution": "node"
    },
    "include": ["index.ts"],
    "exclude": [
        "node_modules"
    ]
}

TypeScript compilation (tsc -p tsconfig.json) works fine. I can also run the script and get the value from that property.

Thank you in advance!

[louisgv]

Here's one instance where the subscription response does not reflect what's actually returned:

const stripe = new Stripe(process.env.STRIPE_SKEY, {
  apiVersion: "2022-08-01"
})

...
  const subscriptionResp = await stripe.subscriptions.list({
    customer: customer.id
  })

  subscriptionResp.data[0].plan?.metadata?.code // <- it error out here as the returned type Subscription does not have `plan`, field however upon inspecting the actual data returned, there was indeed a plan object.

[isaachinman]

@kamil-stripe

The first issue will require some TypeScript trickery, but is possible. Unfortunately, in this case, an array of strings/enum values is probably the worst data structure to work with. You will likely need to instantiate a distinct type for each permutation of the expand array, as per this StackOverflow answer.

While implementing conditional types is necessary for type correctness, and should be implemented here as Stripe is a paid-product, I think the first step is to return the existing types in the first place, which leads into...

The second issue: I already provided the repro, based on the customers.retrieve method. You then wrote a snippet with the subscriptions.retrieve method – not sure why. Here's a codesandbox link.

[kamil-stripe]

Here's one instance where the subscription response does not reflect what's actually returned:

@louisgv This is a separate issue. Please read more at https://github.com/stripe/stripe-node/issues/1517.

[kamil-stripe]

@isaachinman Thanks for the suggestion. I will try to prototype it but considering amount of expandable properties on various resources it may not be easy.

Apologies for missing that the example mentioned the customers endpoint. The endpoint returns a customer or a deleted customer (which doesn't have a created field) and we at the SDK level don't know which one is going to be returned. I will pass the feedback to the service team.

[isaachinman]

@kamil-stripe Great, let me know if you need any advice/pointers. There are far more complex TypeScript projects out there than CRUD SDKs, so this should absolutely be possible.

And, thanks for clarifying about the DeletedCustomer union return type – not sure how I missed that.

For anyone reading this in the future, you can perform type-safe checks like this:

const customer = await stripeClient.customers.retrieve('stripeId')

if ('deleted' in customer) {
  // Customer is deleted
} else {
  // Properties can be accessed safely
  console.log(customer.created)
}

[kamil-stripe]

@isaachinman Definitely! And we want to provide the best experience possible. We just have to think about additional concerns like a property changed by a service team to be non-expandable becoming a breaking change for our SDKs etc. Thanks again for your suggestion.

[guanacone]

I run into a similar TS issue:

When retrieving the list of all prices I expand the response with the product

const { data: prices } = await stripe.prices.list({
    expand: ["data.product"],
});

and I get the data as expected but when I map over the prices and want to use the product name I get an error:

Property 'name' does not exist on type string | Product | DeletedProduct'.
  Property 'name' does not exist on type 'string'.ts(2339)

The error comes form the fact that price.product can be of type string | Stripe.Product | Stripe.DeletedProduct but only Stripe.Product has a property name.

How can I solve this?

[isaachinman]

@guanacone That's a general TS question, and unrelated to this SDK.

[richardm-stripe]

Thank you for the suggestion. I think it's less of a case that the Typescript types are invalid per the title, and more like the Typescript types aren't as narrow as hypothetically they could be.

While this is an interesting idea, I don't think it is likely we will do this. While it might be possible with some trickery to expose narrower types here, this would add complexity to the type definitions and make them more difficult for users to understand. It threatens to make type errors more confusing, and would put more complicated types on the IDE mouseover hints, and so forth.

[isaachinman]

@richardm-stripe That is a disappointing response for a company with the scope and resources of Stripe. I would expect scope-limiting from OSS projects for sure, but declining to accurately type a paid-for product is surprising.

"Trickery" is probably the wrong phrase – it might be "tricky", but all that is required is use of generics.

[richardm-stripe]

Hello @isaachinman, thank you for the feedback.

It less a question of resources, of whether the user experience of the library is better or not with the suggested change. The narrowest type that fits is not always the type that leads to the best user experience.

I've played around a little bit with a proof of concept. I don't think it's possible with just generics -- you need conditional types and a predicate on type-level lists. Something like this.

This is just a proof of concept, but adding this to the library would bloat every return type in the library (every resource would now take a generic argument). It would also lead to confusing type errors (you would have to add as const to the definition of your expand array in order to get the narrowing behavior, and the type error you would get if you forgot does not make it obvious how to fix). This is only the simple case, but I expect things get even more involved when you consider expansions beneath nested objects, and "recursive expansions" (expanding a property inside an expanded object).

[schmod]

As a compromise, I wonder if we could get types for the unexpanded variant of each response?

This would add some bloat to the library, but you could probably accomplish it via "normal" function overloads, ie.

update(
  id: string,
  params?: Omit<SubscriptionUpdateParams, 'expand'> & { expand: never },
  options?: RequestOptions
): Promise<Stripe.UnexpandedSubscription>;
update(
  id: string,
  params?: SubscriptionUpdateParams,
  options?: RequestOptions
): Promise<Stripe.Subscription>;

(The second overload is the "normal" one that we have in the library today)

This would be a huge improvement for many common use-cases, as the current union types are exceedingly difficult to work with.