Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Generating Narrower Types #1538

Closed
harrysolovay opened this issue Sep 22, 2020 · 17 comments
Closed

Generating Narrower Types #1538

harrysolovay opened this issue Sep 22, 2020 · 17 comments
Labels
feature-request New feature or enhancement. May require GitHub community feedback. p2 This is a standard priority issue

Comments

@harrysolovay
Copy link

harrysolovay commented Sep 22, 2020

Is your feature request related to a problem? Please describe.

The smithy-generated TypeScript types can be safer and more inference-producing. For example:

Cognito User Pool's IntiateAuth command accounts for many auth flows (represented by the AuthFlowType enum). Based on that supplied enum, we can discriminate between AuthParameters to type-check flow-specific input fields. We can also return the correct flow-specific outputs (narrowed ChallengeName, ChallengeParameters, AuthenticationResult (its omission), etc.).

For the RespondToAuthChallenge command, here are some of the varying signatures we might get (currently untyped).

const smsMfa = await client.send(new RespondToAuthChallengeCommand({
  AuthFlow: ChallengeTypeName.SMS_MFA,
  ...otherFields,
}));

// {
//   ChallengeName: ChallengeTypeName.SMS_MFA;
//   SMS_MFA_CODE: string;
//   USERNAME: string;
// }


const passwordVerifier = await client.send(new RespondToAuthChallengeCommand({
  AuthFlow: ChallengeTypeName.PASSWORD_VERIFIER,
  ...otherFields,
}))

// {
//   ChallengeName: ChallengeTypeName.PASSWORD_VERIFIER;
//   PASSWORD_CLAIM_SIGNATURE: string;
//   PASSWORD_CLAIM_SECRET_BLOCK: string;
//   TIMESTAMP: string;
//   USERNAME: string;
// }


const newPasswordRequired = await client.send(new RespondToAuthChallengeCommand({
  AuthFlow: ChallengeTypeName.NEW_PASSWORD_REQUIRED,
  ...otherFields,
}))

// {
//   ChallengeName: ChallengeTypeName.NEW_PASSWORD_REQUIRED;
//   NEW_PASSWORD: string;
//   USERNAME: string;
//   attributes: {Name: string; Value: string}[];
// }

Describe the solution you'd like

Let's discuss the approach to smithy-model-to-ts-type codegen. Ideally, we could provide more information, not present in the Smithy models. This info could enable inference of the correct return types based on inputs.

[EDIT]:

If we could specify the input-specific output types like this (for InitiateAuth, in this example):

{
  InitiateAuth: {
    ifInput: [{
      extends: {
        AuthFlow: "AuthFlowType.USER_SRP_AUTH",
      },
      outputConstraints: {
        AuthenticationResult: "undefined",
        ChallengeName: "ChallengeNameType.PASSWORD_VERIFIER",
        ChallengeParameters: {
          SALT: "string",
          SECRET_BLOCK: "string",
          SRP_B: "string",
          USERNAME: "string",
          USER_ID_FOR_SRP: "string",
        },
        SESSION: "undefined",
      },
    },
  }],
}

We could generate a generic output type such as this:

type InitiateAuthCommandOutput<I extends InitiateAuthInput> = I extends {
  AuthFlow: AuthFlowType.USER_SRP_AUTH;
}
  ? {
      AuthenticationResult: undefined;
      ChallengeName: ChallengeNameType.PASSWORD_VERIFIER;
      ChallengeParameters: {
        SALT: string;
        SECRET_BLOCK: string;
        SRP_B: string;
        USERNAME: string;
        USER_ID_FOR_SRP: string;
      };
      SESSION: undefined;
    }
  : WideInitiateAuthOutput;

The InitiateAuth command can keep a generic reference to the constructor param, and pass the narrowed type into $Command.

class InitiateAuthCommand<
  I extends InitiateAuthCommandInput,
  O extends InitiateAuthCommandOutput<I>
> extends $Command<
  InitiateAuthCommandInput,
  O,
  CognitoIdentityProviderClientResolvedConfig
> {
  readonly input: InitiateAuthCommandInput;
  constructor(input: I);
  resolveMiddleware(
    clientStack: MiddlewareStack<ServiceInputTypes, ServiceOutputTypes>,
    configuration: CognitoIdentityProviderClientResolvedConfig,
    options?: __HttpHandlerOptions,
  ): Handler<InitiateAuthCommandInput, O>;
  private serialize;
  private deserialize;
}

The caveat is that the input params need to be narrowed (readonly), but it's still a huge DX improvement for TypeScript power users, and would enable us to safeguard––at the type-level––against all kinds of misuse.

Additional context

To avoid breaking backwards-compat, we could expose the narrowed types as a set of module augmentations, available in @aws-sdk/types/strict.ts.

[EDIT]: moved semi-related (but probably for-now unproductive snippet to comment below)

In Summary

I believe stricter types are essential to customer success, as they serve both in protecting against misuse and ease of experience.

Feedback would be greatly appreciated!

@harrysolovay harrysolovay added the feature-request New feature or enhancement. May require GitHub community feedback. label Sep 22, 2020
@harrysolovay
Copy link
Author

harrysolovay commented Sep 22, 2020

Eventually, it'd be nice to focus on the story around genericization as well.

In certain cases, users may want to supply their own types. DynamoDB document clients, for instance, could accept an Item type param, as to type-check their requests.

import "@aws-sdk/types/strict"; // module augmentations
import {DocDBClient} from "@aws-sdk/client-docdb";

interface Todo {
  id: string;
  title: string;
  body: string;
}

// thanks to the augmentation, we'd be able to specify this generic type param
const client = new DocDBClient<Todo>({
  region: "us-west-2",
});

In the rough example above, passing Todo to DocDBClient narrows the client's allowed operations to the specified data type, ensuring correct IO.

@harrysolovay harrysolovay changed the title Generating Further-narrowed & Generic Types Generating Narrower Types Sep 22, 2020
@harrysolovay
Copy link
Author

Also, can someone possibly explain the generated AuthFlow field type? Why is the AuthFlowType enum "union-ed" with string, as opposed to the narrowed string literals (AuthFlowType[keyof AuthFlowType])? And why is it "union-ed" with undefined, when AuthFlow is required by the service? And... if it could be undefined... why wouldn't AuthFlow be optional?

...
- SomeField: SomeValue | undefined;
+ SomeField?: SomeValue;
...

@trivikr
Copy link
Member

trivikr commented Sep 24, 2020

Also, can someone possibly explain the generated AuthFlow field type? Why is the AuthFlowType enum "union-ed" with string, as opposed to the narrowed string literals (AuthFlowType[keyof AuthFlowType])? And why is it "union-ed" with undefined, when AuthFlow is required by the service? And... if it could be undefined... why wouldn't AuthFlow be optional?

...
- SomeField: SomeValue | undefined;
+ SomeField?: SomeValue;
...

This was done for future-proofing the code, for cases in which required parameters and changed to optional in service types.

Details in #1124 (comment)

[EDIT]
Me (@trivikr) personally don't like the solution of passing | undefined for the same reason of having less strict types. However, ensuring that customers don't have to update SDK version is important.

@harrysolovay
Copy link
Author

I don't feel as though that's the right approach to such future-proofing, as it doesn't take into account other changes to service types (like string -> number, etc.).

Nonetheless, how's this for a solution: we could make stricter types available though module augmentation.

import "@aws-sdk/types/strict";

@trivikr
Copy link
Member

trivikr commented Sep 24, 2020

I don't feel as though that's the right approach to such future-proofing, as it doesn't take into account other changes to service types (like string -> number, etc.).

Changing the type is not allowed under backward compatibility rules.
Making a required type optional is allowed under backward compatibility, and is followed by some service teams.

@trivikr
Copy link
Member

trivikr commented Sep 24, 2020

Nonetheless, how's this for a solution: we could make stricter types available though module augmentation.

import "@aws-sdk/types/strict";

How are stricter types expected to work for specific operations?
For example, the RespondToAuthChallengeCommand operation is imported as follows:

import { RespondToAuthChallengeCommand } from "@aws-sdk/client-cognito-identity-provider";

@harrysolovay
Copy link
Author

@aws-sdk/types/strict/client-cognito-identity-provider

declare module "@aws-sdk/client-cognito-identity-provider" {
  export type RespondToAuthChallengeCommand = NarrowedRespondToAuthChallengeCommand;
}

Just to reiterate...

The generated RespondToAuthChallengeCommandOutput type would be generic (accepting a type which extends RespondToAuthChallengeCommandInput). The RespondToAuthChallengeCommand would be typed such that we'd have the narrowed input type as a type variable, which we could then supply to the RespondToAuthChallengeCommandOutput, thereby narrowing the output type based on the input type. In the example above, this narrowed command is NarrowedRespondToAuthChallengeCommand.

The augmentation would be applied with the following import statement.

import "@aws-sdk/types/strict/client-cognito-identity-provider";

@harrysolovay
Copy link
Author

Loosely (and without the extras):

class RespondToAuthChallengeCommand<
  Input extends RespondToAuthChallengeCommandInput,
  Output extends RespondToAuthChallengeCommandOutput<Input>
> extends $Command<Input, Output> {
  constructor(input: Input) {}
}

@github-actions
Copy link

Greetings! We’re closing this issue because it has been open a long time and hasn’t been updated in a while and may not be getting the attention it deserves. We encourage you to check if this is still an issue in the latest release and if you find that this is still a problem, please feel free to comment or open a new issue.

@github-actions github-actions bot added the closing-soon This issue will automatically close in 4 days unless further comments are made. label Sep 25, 2021
@harrysolovay
Copy link
Author

Commenting so the actions bot doesn't close this out, as I still believe this is an important issue. Hope all is well!

@github-actions github-actions bot removed the closing-soon This issue will automatically close in 4 days unless further comments are made. label Sep 27, 2021
@github-actions
Copy link

Greetings! We’re closing this issue because it has been open a long time and hasn’t been updated in a while and may not be getting the attention it deserves. We encourage you to check if this is still an issue in the latest release and if you find that this is still a problem, please feel free to comment or open a new issue.

@github-actions github-actions bot added the closing-soon This issue will automatically close in 4 days unless further comments are made. label Sep 27, 2022
@harrysolovay
Copy link
Author

Over two years and still no narrowing? How is this possible? Why is there no emphasis on offering a more-narrowly-typed (and therefore safer) DX. Quite disappointing.


For those reading this issue, also disappointed: check out typesafe-dynamodb and franken-srp. typesafe-dynamodb is an absolute gem. The latter––franken-srp––doesn't seem to have caught on... but it's a well-written lib (and the only alternative to Amplify's auth client).

@github-actions github-actions bot removed the closing-soon This issue will automatically close in 4 days unless further comments are made. label Sep 28, 2022
@RanVaknin RanVaknin added the p2 This is a standard priority issue label Feb 17, 2023
@acjay
Copy link

acjay commented Mar 16, 2023

I definitely support at least the option of being able to use stricter types.

Upgrading from V2, I was surprised to find that APIs i was using before, like ParameterStore did not have source direct replacements, and instead, I have to use APIs with types that force me to use postfix ! to override the type checker. As someone using these AWS services directly for the first time, it's a very confusing experience. The lack of documentation around this decision and overall sparse documentation in this library make that even worse. I'm kind of surprised V2 was deprecated with V3 in this state.

I guess if AWS isn't going to fix this, it might just be on the community to write a wrapper with accurate types.

@wesbos
Copy link

wesbos commented Apr 20, 2023

Ran into this today with the union types. Something like MediaFormat or Subtitles.Formats have a clear list of options in the docs, but are just listed as a string in the types. Only once you run the code do you get errors like Member must satisfy enum value set: [amr, flac, wav, ogg, mp3, mp4, webm]

would it not make sense to have these values as a string union instead of just a string?

@kuhe
Copy link
Contributor

kuhe commented Oct 18, 2023

partially addressed in #5356, which closed string unions to only the enumerated values.

@kuhe
Copy link
Contributor

kuhe commented Feb 28, 2024

The service models from which we generate our types do not contain enough information to produce the inference types. We cannot implement that part of the feature request practically.

@kuhe kuhe closed this as completed Feb 28, 2024
Copy link

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.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Mar 15, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
feature-request New feature or enhancement. May require GitHub community feedback. p2 This is a standard priority issue
Projects
None yet
Development

No branches or pull requests

6 participants