Why is everything possibly undefined? (& other typing/doc questions)

[mipearson]

I've been trying to work out how some of these things are intended to be used and having trouble.

Why are all fields possibly undefined?

eg (doc comments removed:

export interface Container {
    reason?: string;
    name?: string;
    exitCode?: number;
    // ...

Is the expectation that we should be putting in guard clauses for every API response, including ones that don't return an error, for things that the APIs are expected to return?

How do we use .config.* properties?

Some code naively ported from V2:

  const ecs = new ECS({});
  const clusters = await ecs.listClusters({});

  if (!clusters.clusterArns) {
    throw new CLIError(
      `Could not find any ECS clusters in ${ecs.config.region}`
    );
  }

ecs.config.region above is typed as Provider<string> | (string & Provider<string>) (which rightfully produces a warning with eslint about embedding it in a template string). Looking at Provider, it looks like it's a promise wrapper. Are we supposed to check via typeof to see if it's a function or a string, and await it if it's a function?

Where are the SDK docs?

I've been relying on intellisense & manually reading the type definitions, but it'd be nice if AWS hosted a typedoc of the various packages.

[rraziel]

One thing I would add to this, the same is true for command inputs, random example:

export interface BatchDetectDominantLanguageRequest {
    /**
     * <p>A list containing the text of the input documents. The list can contain a maximum of 25
     *       documents. Each document should contain at least 20 characters and must contain fewer than
     *       5,000 bytes of UTF-8 encoded characters.</p>
     */
    TextList: string[] | undefined;
}

While the documentation for the API clearly marks that TextList as being required.

[AllanZhengYP]

Hi @mipearson

Regarding the API reference, we just published the v3 API reference here: https://docs.aws.amazon.com/AWSJavaScriptSDK/v3/latest/index.html

We are still working on to polishing it, please feel free to open issues if you have any questions or feedbacks.

[rraziel]

Taking a closer look, and with CreateFunctionRequest as an example:

// Note: not from the code
type CreateFunctionRequest = {
    Handler?: string;
    Code: FunctionCode | undefined;
};

In practice, Handler is mandatory and Code is optional.

Would it be possible to generate code that reflects it?

type CreateFunctionRequest = {
    Handler: string;
    Code?: FunctionCode;
};

[rraziel]

Which seems to come from here: https://github.com/awslabs/smithy-typescript/blob/master/smithy-typescript-codegen/src/main/java/software/amazon/smithy/typescript/codegen/StructuredMemberWriter.java#L64

String optionalSuffix = shape.isUnionShape() || !isRequiredMember(member) ? "?" : "";
String typeSuffix = isRequiredMember(member) ? " | undefined" : "";

It's really not convenient the way it works right now, e.g.:

// given this (where Handler is @required and Code is optional)
type CreateFunctionRequest = {
  Handler?: string;
  Code: FunctionCode | undefined;
};
// you can do this:
const test: CreateFunctionRequest = {
  Code: undefined
};

And even if you pass Handler, you cannot omit Code and have to explicitly set it to undefined.

[andcea]

I've come across this issue as well while using the textract client.

For example:

export interface BoundingBox {
    Width?: number;
    Height?: number;
    Left?: number;
    Top?: number;
}

That object cannot logically exist without all those fields defined.

If you're using typescript this causes code that is much more unnecessarily verbose, having to check all the members for undefined.

[mipearson]

Yeah, I end up abusing ! a fair bit when working with AWS SDK responses - or co-ercing what it returns into safer types.

[rraziel]

This is what my sources are littered with:

// FIXME: https://github.com/aws/aws-sdk-js-v3/issues/1613
type BatchDetectDominantLanguageCommandInputFixed = BatchDetectDominantLanguageCommandInput & {
  TextList: NonNullable<BatchDetectDominantLanguageCommandInput['TextList']>;
}

(which could be turned into a = FixedAwsType<BatchDetectDominantLanguageCommandInput, 'TextList'> but that would mean accepting it'll never be fixed 😄 )

[allan2]

I just wanted to mention that HostedZone has string | undefined type for the field id. There are other fields affected.

The Go API Reference states that that fields are required. Perhaps parsing that reference could eliminate undefined unions.

Is there any motivation for keeping the undefined union in fields that are SDK required?

[Woodz]

This is the same issue as aws/aws-sdk-js#1553 which was not addressed in v2 JS SDK. It looks like AWS don't clearly document which fields are mandatory, particularly on output models, so the SDK just presents all fields as optional. This introduces a lot of risk of potential bugs (should we be expected to check that each field in the output model is defined?) and significantly hurts developers working in TS. To see such a major bug unresolved for almost 5 years is concerning

[G-Rath]

I've raised this issue myself before, and was told:

Using | undefined is a thought out decision explained in #1124 (comment)

[Woodz]

Ridiculous! Making all output fields optional so that they can remove them in future versions without breaking the API is just wrong. If the field is important, I will need to change my code when you stop sending the field - this is a breaking change in my app. By making important output fields optional you are actually making things worse because my code will still compile when you stop populating it!

I'm going to have to use a non-null assertion every time I use an AWS output field and my downstream code will just break if/when they stop sending it. I have no other choice (the only other option I have is to check whether an important field is null and it is, all I can do is throw an error).

[rraziel]

So, coming back to this topic, I understand there is a rationale behind the current behavior (and changing it would be a breaking change).

How about having a flag that can be passed when doing a cdktf get (or cdktf.json or an environment var) so that it generates the TS with this other model people are asking for?

[AllanZhengYP]

Hi @rraziel This @trivikr's comment explains the rationales of the undefined. That is for the current status, we are weighing possible new solutions internally to reduce the undefined.

[lumattr]

Ridiculous! Making all output fields optional so that they can remove them in future versions without breaking the API is just wrong. If the field is important, I will need to change my code when you stop sending the field

As an extension of this, This is the whole point in using versioning in the first place. As consumers we lock to particular versions to ensure compatibility. When we upgrade we acknowledge that the code we are using may change and that our code will need to change to accomodate it. By saying "well we may change the code in the future" you may as well save yourselves the trouble and just distribute a single version of the code as theres no benefit to having versioning behind this reasoning.

Your even including fields that are inherently required by the backend for it to be able to function, which doesnt make sense. My example being the ListTagsForResourceCommandInput. That function has a requirement for a resource arn parameter, even though the input parameter to be undefined. Pretty much defeats the purpose of a strongly typed language. The DescribeLogGroupsCommandOutput even returns a LogGroup array where even field is optional. The backend cant function without having an ARN, so why are we expected to validate its existense.

[SimonSchick]

Also chiming in, currently migrating to @aws-sdk/* and this is a MAJOR pain, why was this done? This almost seems like someone saw protobuff and only took the bad parts.

You segmented clients into smaller packages, if the api changes make breaking changes, don't create completely arbitrary api constraints just so you can claim 'technically not a breaking change :)' later.

[Cameramorphic]

I just encountered this too, using EventBridge Scheduler and trying to update an existing schedule.
As mentioned on the UpdateScheduleCommand page, it is recommended to fetch the schedule first with GetSchedule to avoid resetting to defaults on update. But in the output of GetScheduleCommand everything is optional, even the fields required on create/update. It is not even using the previously mentioned (equally questionable) pattern of "required with | undefined" as declared in the input types.
Now i can't just spread ...fetchedSchedule into the update input, but I could manually overwrite the required fields with the same possibly undefined value from fetchedSchedule, which would still fail if it actually was undefined...
As everyone else mentioned, this is terrible!

Also, why does e.g. the UpdateScheduleCommand page show which input fields are required, but the linked UpdateScheduleCommandInput page doesn't?

[RanVaknin]

Hi everyone on the thread.

To improve the typescript experience, we added a new feature, you can now cast your client to be an UncheckedClient or AssertiveClient from Smithy-Typescript package. Just be mindful that using UncheckedClient eliminates compiler-generated nullability warnings for output fields by treating them as non-nullable. You should still perform manual type checks as necessary for your application's specific needs.

further discussion on this was done here #5992

Since this should address the original concern of removing the " | undefined" I'm going to close this issue and also lock it.
I'm locking it because we do not monitor closed issues and do not want to miss any correspondence. If this is still a problem, we ask that you open a separate new issue so we can better assist you.

Thanks again for your patience and for staying active.
All the best,
Ran~