-
Notifications
You must be signed in to change notification settings - Fork 64
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: add a required input prompt for use in region input #2292
Conversation
🦋 Changeset detectedLatest commit: 4a57977 The changes in this PR will be included in the next version bump. This PR includes changesets to release 2 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
packages/cli-core/API.md
Outdated
static requiredInput: (options: { | ||
message: string; | ||
}) => Promise<string>; |
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.
how about
static input: (options: {
message: string;
defaultValue?: string;
required?: boolean
}) => Promise<string>;
above?
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.
(or some type union for options
to make default and required mutually exclusive)
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.
combining it was making it slightly cryptic without making a break in the API.
e.g.
{
message: string;
required: false;
defaultValue?: string;
} | {
message: string;
required: true;
}
is a breaking change and
{
message: string;
defaultValue?: string;
} | {
message: string;
required: true;
}
allows both defaultValue
and required
shows up in the intellisense which is confusing API
await AmplifyPrompter.input({
message: `Hit [enter] when complete`,
required: true,
defaultValue: ''
});
I figured having an explicit API is probably better to not touch the existing uses of input
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.
I did see this pattern in bedrock SDK. might be worth trying.
{
message: string;
required?: never;
defaultValue?: string;
} | {
message: string;
required: true;
defaultValue?: never;
}
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.
ohh that's neat. Updated.
Problem
Currently we accept "Enter" as acceptable input for region while configuring the profile. This results in
""
value to be stored as region which later fails validationChanges
fix: add a required input prompt for use in region input
Corresponding docs PR, if applicable:
Validation
Checklist
run-e2e
label set.By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.