-
Notifications
You must be signed in to change notification settings - Fork 816
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
fix: improved typing for $TSContext #11962
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is going into right direction I think.
d37715b
to
6caf178
Compare
6942755
to
fc34cc9
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The cli-internal - cli-core moves are looking good.
I am concerned about moving auth types to core.
...sts__/provider-utils/awscloudformation/auth-stack-builder/auth-cognito-stack-builder.test.ts
Outdated
Show resolved
Hide resolved
...egory-auth/src/__tests__/provider-utils/awscloudformation/handlers/resource-handlers.test.ts
Outdated
Show resolved
Hide resolved
This reverts commit 5c3b053.
...ategory-auth/src/provider-utils/awscloudformation/auth-stack-builder/auth-stack-transform.ts
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A couple small comments but overall looks like a great step in the right direction!
...sts__/provider-utils/awscloudformation/auth-stack-builder/auth-cognito-stack-builder.test.ts
Outdated
Show resolved
Hide resolved
command: string; | ||
subCommands?: string[]; | ||
options?: { | ||
restore?: boolean; | ||
json?: boolean; | ||
name?: string; | ||
awsInfo?: string; | ||
config?: string; | ||
'iterative-rollback'?: boolean; | ||
force?: boolean; | ||
env?: string; | ||
rootStackName?: string; | ||
frontend?: string; | ||
quickstart?: boolean; | ||
app?: string | boolean; | ||
timeout?: string; | ||
event?: string; | ||
minify?: boolean; | ||
help?: boolean; | ||
localEnvFilePath?: string; | ||
yes?: boolean; | ||
appId?: string; | ||
} & Record<string, string | boolean>; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Another vote in favor of defining this type as Record<string, string | boolean>
Description of changes
Adds additional typing for $TSContext.
As a general rule, types were not renamed unless there was a logical conflict or additional clarity was needed.
All types/interfaces used to define $TSContext were moved into
amplify-cli-core
.In many cases across the codebase, nullable values are treated as non-nullable. In cases where this occurred, the type was defined as nullable, and null checks were added where needed. In cases where non-null assertions were removed and a null value would throw a type error in the existing code, a null check is added that specifically throws a type error.
Where possible, types and interfaces were moved to
amplify-cli-core
, and implementers are left inamplify-cli
(or-auth
).Issue #, if available
Description of how you validated changes
Checklist
yarn test
passesBy submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.