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

Add types package #4

Merged
merged 4 commits into from
Apr 28, 2017
Merged

Add types package #4

merged 4 commits into from
Apr 28, 2017

Conversation

jeskew
Copy link
Contributor

@jeskew jeskew commented Apr 12, 2017

No description provided.

@jeskew jeskew requested a review from chrisradek April 12, 2017 20:48
@jeskew jeskew force-pushed the feature/add-types-package branch from 4ae18ba to 2b9a393 Compare April 14, 2017 20:44
@jeskew jeskew force-pushed the feature/add-types-package branch from bc79a1e to 38414d0 Compare April 17, 2017 20:29
Copy link
Contributor

@chrisradek chrisradek left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How is this package supposed to be used?

This looks like you've created interfaces that more or less describe the contents of a service model.

Are these types supposed to be used by the code generator so that it knows what a model looks like, or is the code generator creating objects from the model that adhere to these interfaces? Or is it something else entirely?

I get that other interfaces, like Credentials, are stored here so that we can import them in other packages, it's just the Shape/Operation ones that I'm unclear about.

/**
* AWS secret access key
*/
readonly secretKey: string;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What about calling this secretAccessKey? Customers will be used to that term today, and it's what our INI creds and ENV var creds use.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated.


export interface String extends Shape {
type: 'string';
min?: number;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we'll also need to model the custom traits.

jsonvalue?: boolean
idempotencyToken?: boolean

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added

}

export interface OperationModel {
http: HttpTrait;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There are also some operation custom traits.

authtype?: 'none'|'v4'|'v4-unsigned-body', maybe string as well (Depending on how we handle some cases)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we punt on this one? I was kinda planning on using the authtype to bind a handler onto command objects instead of including it as a string in the operation model, but we haven't discussed the design of that yet.

@jeskew
Copy link
Contributor Author

jeskew commented Apr 27, 2017

The service model types are meant to describe the content of a generated serialization model rather than a service model proper. The model is expected to be normalized (every operation has an input, output, and errors, and some types (like integers, floats, bytes, longs, characters, etc.) are collapsed into simplified types), but it is very close to how services are defined now.

Serialization models, because they use shapes that may be recursive, require type definitions to be provided -- typescript won't be able to infer the type of a DynamoDB AttributeValue, so not defining it as a "structure" is an implicit any typing. The shape definitions would be used by all generated serialization objects, and those are expected to appear in 90+ packages.

@jeskew jeskew force-pushed the feature/add-types-package branch from e8f8821 to d6d532c Compare April 28, 2017 00:25
Copy link
Contributor

@chrisradek chrisradek left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Merge it in!

@jeskew jeskew merged commit a6a9312 into master Apr 28, 2017
@jeskew jeskew deleted the feature/add-types-package branch April 28, 2017 22:07
trivikr referenced this pull request in trivikr/aws-sdk-js-v3 Dec 10, 2018
@lock
Copy link

lock bot commented Sep 27, 2019

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.

@lock lock bot locked as resolved and limited conversation to collaborators Sep 27, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants