Remove | string from enums

[everett1992]

Describe the feature

smithy-typescript and aws-sdk-js-v3 generate clients with | string for any enum type in the model. It breaks auto complete, which is annoying, and compile time type checks, which is unsafe.

Use Case

TypeScript simplifies the documented ¹ type from ExecutionStatus | string to string in error messages, auto complete, and type hints so it's hard to discover the generated enum values from an editor or commandline.

The inclusion of string in output types prevents me from writing exhaustiveness checks that would fail at compile time if my code did not handle all cases. For example handling step functions execution status¹.

// According to aws-sdk docs[^1] the type is ExecutionStatus | string | undefined
// but according to typescript the type is `string | undefined`.
//  ExecutionStatus | string is simplified to `string`.
const {status} = await sfn.describeExecution({executionArn})

switch (status) {
  case ExecutionStatus.ABORTED:
  case ExecutionStatus.FAILED:
  case ExecutionStatus.TIMED_OUT:
    return false;
  case ExecutionStatus.RUNNING:
    return undefined;
  case ExecutionStatus.SUCCEEDED:
    return true;
    console.log(`some known status ${status}`)
  case undefined:
    // https://github.com/aws/aws-sdk-js-v3/issues/1613
    throw Error('undefined')
  default:
    // This is a type error because status could be any string other than the enumerated values we checked above.
    assertNever(status)       ● Argument of type 'string' is not assignable to parameter of type 'never'.
}

The inclusion of string in input types allows me to pass invalid values²

await sfn.createStateMachine({
  // no type error here :(
  type: 'nope',
  ...args
})

I understand that smithy requires³ clients to allow unmodeled enum values but it does not require the current implementation.

Proposed Solution

My proposal is to replace all | string with a specific boxed value Unmodeled<string>. This is inspired by rust aws-sdk⁴, but obviously typescript doesn't support parameterized enums like they do.

Instead of status: ExecutionStatus | string (which is simplified to string) we would have status: ExecutionStatus | Unmodeled<string>. aws-sdk can ship a isModeled typeguard which can narrow to only known values, or handle all unknown values.

// just one way to implement the box
const unmodeled = Symobol.for('unmodeled')
interface Unmodeled<T> { value: T, [unmodeled]: true }

function isModeled<T>(value: T | Unmodeled<unknown>) value is T {
  return !(unmodeled in value);
}

The sdk would still handle unknown enum values in respondes, but any value not modeled at build time would be wrapped with Unmodeled. Callers could still provide unmodeled values as long as they wrap the value, providing type safty for the majority of uses.

await sfn.createStateMachine({
  // no type error here :)
  type: Unmodeled('nope'),
  ...args
})

I think this pattern could be used beyond enums and replace | undefined and ? added to inputs. I do not recommend using this pattern for optional outputs #1613, where I prefer optional chaining.

Acknowledgements

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

SDK version used

3.229.0

Environment details (OS name and version, etc.)

N/A

Footnotes

1. https://docs.aws.amazon.com/AWSJavaScriptSDK/v3/latest/clients/client-sfn/interfaces/describeexecutioncommandoutput.html#status
   https://github.com/aws/aws-sdk-js-v3/blob/main/codegen/sdk-codegen/aws-models/sfn.json#L3479 ↩ ↩²

2. https://docs.aws.amazon.com/AWSJavaScriptSDK/v3/latest/clients/client-sfn/interfaces/createstatemachinecommandinput.html#type
   https://github.com/aws/aws-sdk-js-v3/blob/main/codegen/sdk-codegen/aws-models/sfn.json#L5890 ↩

3. Enums are considered open, meaning it is a backward compatible change to add new members. Previously generated clients MUST NOT fail when they encounter an unknown enum value. Client implementations MUST provide the capability of sending and receiving unknown enum values.
   https://smithy.io/2.0/spec/simple-types.html#enum-is-a-specialization-of-string

   ↩

4. https://docs.rs/aws-sdk-sfn/latest/aws_sdk_sfn/model/enum.ExecutionStatus.html ↩

[everett1992]

I have found a useful way to narrow enums to their modeled values, but it requires that you know the enum type. And it does not help type safety on inputs.

function isModeledEnum<T>(enumType: { [s: string]: T }, v: T | string | undefined): v is T {
  return Object.values(enumType).includes(v as any)
}

const {status} = await sfn.describeExecution({executionArn})
if (!isModeledEnum(ExecutionStatus, status) throw Error()
// status is now ExecutionStatus

[RanVaknin]

Hi @everett1992 ,

The reason why we are generating enums with the | String is for forward compatibility reasons. Often times service teams release new features but don't go through the Trebuchet release process causing officially released features to not exist in the SDK. I personally deal with filing internal tickets to teams to fix modeling issues of this sort, and it's not sustainable, there are just too many discrepancies.

This is our measure of ensuring even if there's no enum types generated, customers could still have a meaningful way to handle enums.

We have discussed this as a team and decided we are not going to change it. If you have some other thoughts I encourage you to write us internally so we can discuss this further.

Thanks!
Ran

[everett1992]

The rust SDK generates enums for known values and an extra Unknown value for forward compat.
My proposal is to do something similar and generate a Unknown<string> box

#[non_exhaustive]
pub enum ExecutionStatus {
    Aborted,
    Failed,
    Running,
    Succeeded,
    TimedOut,
    Unknown(UnknownVariantValue),
}

type ExecutionStatus = 
  |  Aborted,
  |  Failed,
  |  Running,
  |  Succeeded,
  |  TimedOut,
  |  Unknown<string>,
  
const res = await sfn.createStateMachine({
  status: unknown("An unmodeled value for execution status"),
})

if (typeof res.status === 'string') {
  // res.status is one of the modeled values
} else {
  // we can access the unknown value
  res.status.unknownValue
}

[nmussy]

FWIW, the TypeScript 5.2 Iteration Plan plans to alleviate this issue with microsoft/TypeScript#29729

[everett1992]

microsoft/TypeScript#29729 only addresses autocomplete which is a small part of the issues | string causes.

• I want status: "TIMD_OUT" to be an error (typo)
• I want switch (execution.status) to have exhaustive checks and error if I don't cover a modeled value
• I want aws-sdk's own documentation not to show DescribeExecutionCommandOutput's status as string | undefined

[mdpratt]

So @mattpocock shares an excellent workaround to allowing autocomplete with defined types, but also allow any generic string. He covers it here: https://www.youtube.com/live/8HoOxOd86M4?feature=share

TLDR: Change | string to | (string & {})

// This will allow autocomplete for the types, AND still allow generics. 🤩
type Keys = "Active" | "Inactive" | "InProgress" | (string & {});

// Autocomplete shows  "Active" | "Inactive" | "InProgress"
// But any generic string is allowed
const value: Keys = "Deleting"

You can try it out over on the TS Playground, just go to the "" and hit Ctrl/Cmd+Space to see the autocomplete.

[mattpocock]

@mdpratt

This is the way.

[kuhe]

Could you redirect this to the Smithy team either internally or in their repository: https://github.com/awslabs/smithy-typescript/tree/main?

This is controlled in the file:

https://github.com/awslabs/smithy-typescript/blob/main/smithy-typescript-codegen/src/main/java/software/amazon/smithy/typescript/codegen/SymbolVisitor.java

In my opinion the | string should be removed entirely.
Users wanting to override (e.g. using an enum not present in their installed version) should simply cast or remove typecheck on their code expression.

For switches, a default seems to be acceptable even when all enum options are exhaustively checked.

[github-actions]

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs and link to relevant comments in this thread.