`Stripe.Metadata` has incorrect type

[SpencerKaiser]

trafficstars

Describe the bug

The type for Stripe.Metadata is { [name: string]: string; } which is incorrect; this specifies that all keys have a value of string when in reality, any key could be string OR `undefined.

To Reproduce

const { wat } = {} as Stripe.Metadata;

Expected behavior

Any valued pulled from a metadata object should have the type string | undefined

Code snippets

typescript
const { wat } = {} as Stripe.Metadata;

### OS

macOS

### Node version

Node v16.13.0

### Library version

stripe 9.16.0

### API version

'2020-08-27'

### Additional context

Fixing this will be a breaking change for users who consume this type in typescript

[kamil-stripe]

Hey @SpencerKaiser! Thank you for your report. I'd like to know more about your use case. Are you able to share it with us? I am also confused with the proposed code change. You mention in "Expected behavior" section that any value should be treated as string | undefined but the pull request changes the type of the key. Would https://www.typescriptlang.org/tsconfig#noUncheckedIndexedAccess be helpful in your case?

Lastly, the pull request does not build properly. For the proposed change, the TypeScript compiler fails with following message"

';' expected.
Property or signature expected.

See TS playground to reproduce.

If we adjust the code to following:

interface Metadata {
    [name: string | undefined]: string;
}

it results in

An index signature parameter type must be 'string', 'number', 'symbol', or a template literal type.

Let me know what you think. Looking forward to your input.

[SpencerKaiser]

@kamil-stripe 👋 noUncheckedIndexedAccess looks relevant, but I wouldn't necessarily want to enable that just to address this one use case. And my bad on the pr 🤦‍♂️ I actually think [name: string]: string | undefined is what I wanted there... thoughts?

Regarding our exact use case, we wanted to provide a means for unauthenticated users to purchase home service gifts without being authenticated (shameless plug for that functionality 🤪). Per the docs, we aren't saving payment intents and are instead relying on metadata to provide context. One of the metadata values we used was serviceId, which is included when a user submits specifies an exact service for the gift, but that field is optional in our system so it will get trimmed off the request to Stripe if we don't explicitly set it to null instead of undefined. When we receive the webhook event from Stripe, we try and pull that value out of the metadata and then retrieve info about that service. Because the type was string instead of string | undefined our team member didn't catch a potential bug when we query a table by that ID.

Original code (obfuscated):

const { serviceId /* other values */ } = metadata;

const service = await db.findOne(Service, { id: serviceId }); 
// 💥 Runtime error; Malformed SQL query

Our "patch":

const { serviceId /* other values */ } = metadata as Partial<Stripe.Metadata>;

const service = await db.findOne(Service, { id: serviceId }); 
// 💥 Compile time error: Type 'undefined' is not assignable to type 'string'.

TL;DR - I think it's safer to force devs to handle optional properties on ALL metadata values because it's impossible to force strong types on it. As a result, errors should be shifted to compile time more often vs runtime.

[kamil-stripe]

@SpencerKaiser I see. Thank you for the detailed explanation and congrats on the new functionality. Let me discuss this with the broader team and get back to you.

[pakrym-stripe]

Hi, @SpencerKaiser.

I think the noUncheckedIndexedAccess is the way to go here. The metadata dictionary has the same semantics as Record<string, string> which doesn't return undefined by-default .

noUncheckedIndexedAccess is a feature specifically designed to address your use case.

[SpencerKaiser]

@pakrym-stripe it's a neat flag for sure, but I still don't think the type is accurate the way it is. Given the nature of Metadata there will never be a guarantee that anything in it is a string; why not be explicit and say that the keys inside that object could be strings or non-existent?

Plus for the average user, flipping that flag to true is gonna cause some type errors and I doubt yall want to add documentation encouraging people to turn that flag on... just seems like the safe bet here is to change the type to be "safe" for all situations.

[pakrym-stripe]

Given the nature of Metadata there will never be a guarantee that anything in it is a string; why not be explicit and say that the keys inside that object could be strings or non-existent?

The same is true for every other use of Record<string , X> in TypeScript (this type checks). Stripe library just follows the stablished pattern.

If customers want to make indexing untyped dictionaries be a strict operation there is a standard flag for that (same example produces an error).

Can you help me understand how Metadata dictionary is different from other places that will trigger an error when you enable noUncheckedIndexedAccess ?

[SpencerKaiser]

The difference is that the user isn't creating that type as part of their project; it's an established component of your SDK. When I use SDKs I expect that the maintainers will have created types that prioritize safety over everything else and I consider the type of that attribute to be unsafe because the layman user and the pro user alike will see the indexed value to be of type string when in reality there is no guarantee, unless they set a config flag that they may not even be aware of (I wasn't until @kamil-stripe pointed it out! 😅).

If I were creating my own Record<string, X> type in my app, I'm 100% at the mercy of that type and I would either flip that flag on OR set the type to be X | undefined if it was important to me. With the Stripe SDK, I don't have control over it and aside from using something like patch-package to make the change myself, the only other options I have are to expect my developers to be 100% certain the value will be there (which is a risk) OR enable that flag for my entire project and deal with the consequences.

If yall make the proposed change, 100% of your users (regardless of their config) will have to address the possibility that the value in metadata may be undefined, which is a safer practice which will likely result in fewer bugs for your customers, so it seems like a win-win.

[pakrym-stripe]

I've chatted with the team and we decided to follow what's normally done in TypeScript ecosystem and not change the definition of Metadata.

Some reasons:

1. The result of Object.fromEntries seems like a good analog of metadata and it's type as {[k: string]: string}:

2. {[k: string]: string | undefined} allows for

{
    a: "a",
    b: undefined
}

which is not correct.

3. {[k: string]: string | undefined} makes Object.values() incorrectly typed as (string|undefined)[]
4. We didn't hear other customers confused by the type.
5. There is standard way to make the indexer typing stricter.

[SpencerKaiser]

Full disclosure: I made the change to my TSConfig and have changed my opinion to a certain degree. That being said, some feedback on those points:

1. Sure, but I'd argue it's the exact same issue... it just shifts the blame to TypeScript, which I think is fair.
2. Writing to metadata and reading from it are two different problems. I agree writing undefined into it is not correct, but the "value" coming back from it can absolutely be undefined because the key might not be there... I imagine this would be a practical use case for having a type the API calls and a type for the webhooks that have subtle differences to match the reality of how they're used... which is why yall have MetadataParam which doesn't overlap with Metadata
3. I agree the value there is misleading with Object.values. That being said, I think it's far more likely someone will destructure the object than arbitrarily pull all values from it into an array, so I still think the change would be a net positive for type safety.
4. I mean... you're probably not going to, they're just going to get bugs and be frustrated. This issue was primarily targeted to assist junior to mid-level devs and they're probably the least likely to get involved with issues on GitHub.
5. Fair, but I think adding a recommendation to your docs for this would be very helpful for this exact reason... if yall are open to this, I'd be happy to create a PR to add it as a small bullet to the Usage with TypeScript section

💡 TL;DR - For anyone who stumbles upon this and wants to make their types afety a bit safer, just add "noUncheckedIndexedAccess": true to your tsconfig.json and all values in .metadata will have the type string | undefined to reflect that the key may be missing.

patch-package option for anyone who can't/won't use "noUncheckedIndexedAccess": true

1. Add patch-package as a dependency (npm i patch-package or yarn add patch-package)
2. Go to your node modules and open stripe/types.shared.d.ts
3. Modify line 7 (the implementation for Metadata to match this:

 interface Metadata {
  [name: string]: string | undefined;
}

4. Run npm patch-package stripe (or yarn patch-package stripe)

Any time you run npm i or yarn, your patch will be applied and the type of Metadata will be updated to help prevent bugs associated with missing keys