TypeScript - mark response properties required if they're guaranteed to be present

[jedwards1211]

Describe the feature

It would be way better if not every response property was optional. I'm sure there were reasons for this, but I'm complaining anyway -- are the reasons for it really that justified? ಠ_ಠ I doubt it, I think AWS could invest in making it better. Overall the SDK is pretty great to work with, but the blanket optional properties waste a lot of time.

Use Case

All response properties being optional makes TypeScript code cantankerous and ugly:

const { Stacks: [Stack] = [] } = await cfn.send(new DescribeStacksCommand({ StackName })

if (!Stack) {
  throw new Error(`this shouldn't happen, but we need a non-null assertion here`)
}

const Parameters = Object.fromEntries(
  // it's silly for ParameterKey to be marked optional
  Stack.Parameters?.flatMap(p => p.ParameterKey ? [[p.ParameterKey, p.ParameterValue]] : [])
    || []
)

Proposed Solution

I don't know why exactly this is (microservices being implemented in a language where all values are nullable or something? Some obscure option to select which properties of the response we want?) But, there has to be a way to write up metadata on which properties are guaranteed to be defined in service responses.

Other Information

You might say, why not use TypeScript non-null operations (the trailing !), but it feels too dangerous. It's reasonable to assume an ARN or Id property will always be defined, but there are plenty of properties where I don't think I can say for sure what the API guarantees.

Acknowledgements

• [ ] I may be able to implement this feature request
• [ ] This feature might incur a breaking change

SDK version used

3.588

Environment details (OS name and version, etc.)

macOS any

[tracy-french]

+1.

Every field of a response seems to be set as a union with undefined. This typically leads to boiler-plate defaulting code on responses for fields which are actually required. The same is true for response types, which can lead to bugs, as you can end up sending an invalid request fairly easily.

I've been overcoming this limitation using type-fest's SetNonNullable utility type. It removes the undefined unions while retaining the ? optional annotations. For example:

import type { DescribeProjectResponse } from '@aws-sdk/client-iotsitewise';

type ActualDescribeProjectResponse = SetNonNullable<DescribeProjectResponse>;

The SDK response is typed as:

interface DescribeProjectResponse {
  portalId: string | undefined;
  projectArn: string | undefined;
  projectCreationDate: Date | undefined;
  projectDescription?: string | undefined;
  projectId: string | undefined;
  projectLastUpdateDate: Date | undefined;
  projectName: string | undefined;
}

After using SetNonNullable:

interface ActualDescribeProjectResponse {
  portalId: string;
  projectArn: string;
  projectCreationDate: Date;
  projectDescription?: string;
  projectId: string;
  projectLastUpdateDate: Date;
  projectName: string;
}

The fields are now correctly typed, with projectDescription retaining the optional annotation.

It's not a perfect utility, as it's not recursive. For a List* API with an array field, you would need to do the same on the array elements. You also need to cast the responses returned to the correct types, which makes me a bit nervous.

I'm the owner of the API in my example, so I feel pretty confident about the ActualDescribeProjectResponse type being correct and I don't understand why the SDK would use the undefined union pattern. Could someone provide some clarity for why this decision was made?

I imagine it would be a major breaking change to remove the undefined unions, so I don't expect that to happen. It would be cool if we could add a second set of types with the correct typings, or a "strict" mode on the SDK for the methods/commands to return the correct types.

Thank you! :)

[RanVaknin]

Hi there,

I have covered this in my response here https://github.com/aws/aws-sdk-js-v3/issues/5992#issuecomment-2052253822

Please let us know if you have any other questions. Thanks, Ran~

[jedwards1211]

Thanks @RanVaknin.

I understand the forward compatibility goals but I'm not sure how well that actually accomplishes it. There are many processes we have to write that can only continue if an id, ARN, or other field is present in a response. If AWS made one of those fields optional in the future, even if our process checks for that and avoids errors, it would have to abort without being able to accomplish its intended purpose, so the change would be effectively breaking anyway.

So I'd like to see AWS commit to at least guaranteeing they're not going to make ARN or id-like fields optional without warning (which would include things like RoleName or InstanceProfileName that are the only way many commands allow us to uniquely identify a resource). If a response field like that becomes optional it should be released as a noticeable breaking change.

[jedwards1211]

Also, where do the docs live on GitHub now? I could make a PR to reference the smithy types you pointed out. (The documentation source link in the developer guide points to the archived https://github.com/awsdocs/aws-sdk-for-javascript-v3)

[kuhe]

Our recommendation is to use one of the type transforms from @smithy/types.

https://docs.aws.amazon.com/AWSJavaScriptSDK/v3/latest/Package/-smithy-types/

import { S3 } from "@aws-sdk/client-s3";
import type { AssertiveClient, UncheckedClient } from "@smithy/types";

const s3a = new S3({}) as AssertiveClient<S3>;
const s3b = new S3({}) as UncheckedClient<S3>;

// AssertiveClient enforces required inputs are not undefined
// and required outputs are not undefined.
const get = await s3a.getObject({
  Bucket: "",
  Key: "",
});

// UncheckedClient makes output fields non-nullable.
// You should still perform type checks as you deem
// necessary, but the SDK will no longer prompt you
// with nullability errors.
const body = await (
  await s3b.getObject({
    Bucket: "",
    Key: "",
  })
).Body.transformToString();

This differs from SetNonNullable in that it only removes | undefined from required fields, and is recursive.

[jedwards1211]

@kuhe Yes, I found that in the linked comment. However, that information wasn't discoverable enough for me to find it before I opened an issue; I think we should add it to the Developer Guide for SDK Version 3, because I would have found it there.

Aside from that, I want to know if y'all have any specific response about this:

There are many processes we have to write that can only continue if an id, ARN, or other field is present in a response. If AWS made one of those fields optional in the future, even if our process checks for that and avoids errors, it would have to abort without being able to accomplish its intended purpose, so the change would be effectively breaking anyway.

So I'd like to see AWS commit to at least guaranteeing they're not going to make ARN or id-like fields optional without warning...

And by commit I mean at least mark those fields required.

[kuhe]

We can make a request to our documentation writer teammates to add a link to the supplemental developer-written docs. @RanVaknin please open a ticket.

Our default types have | undefined on fields that are "required" as defined in the service models. We are not likely to remove this because of backwards compatibility, and to encourage runtime type-checking of service responses coming from outside the SDK application code. That is why the type transform is an opt-in addon instead of default behavior.

We may decide to remove | undefined in the future, since it does cause a fair bit of confusion, I think, but it's not currently planned, since it would require a long campaign to make users aware of the breaking change. We are also, as a matter of policy, unable to release a major version update to the AWS SDK at this time.

[jedwards1211]

@kuhe it would be nice to have a way to opt into types that are kind of a middle ground, where response properties like arns, ids, names used to uniquely identify things, tag and parameter keys etc. are required, but there's still | undefined on other properties where there's more of a risk that AWS would make a breaking change.