-
Notifications
You must be signed in to change notification settings - Fork 1k
Fix issue where projectId and displayName are not getting parsed #8635
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
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.
LGTM with some type nits.
src/init/features/project.ts
Outdated
@@ -41,12 +41,12 @@ function toInitProjectInfo(projectMetaData: FirebaseProjectMetadata): InitProjec | |||
}; | |||
} | |||
|
|||
async function promptAndCreateNewProject(): Promise<FirebaseProjectMetadata> { | |||
async function promptAndCreateNewProject(options: any): Promise<FirebaseProjectMetadata> { |
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 should probably be options: Options, right?
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.
Thanks for the feedback! didn't know we had an Options
type, will update!
src/management/projects.ts
Outdated
}, | ||
}); | ||
export async function promptProjectCreation( | ||
options: any, |
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.
Same, probably should use Options as the type here.
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.
It looks like using the Options
type raises and error
I'm guessing this is because displayName
is not a property in either Options
or BaseOptions
, so displayName
gets cast as unknown
in [key: string]: unknown;
Lines 6 to 41 in d965588
export interface BaseOptions { | |
cwd: string; | |
configPath: string; | |
only: string; | |
except: string; | |
config: Config; | |
filteredTargets: string[]; | |
force: boolean; | |
// Options which are present on every command | |
project?: string; | |
projectAlias?: string; | |
projectId?: string; | |
projectNumber?: string; | |
projectRoot?: string; | |
account?: string; | |
json: boolean; | |
nonInteractive: boolean; | |
interactive: boolean; | |
debug: boolean; | |
rc: RC; | |
// Emulator specific import/export options | |
exportOnExit?: boolean | string; | |
import?: string; | |
isMCP?: boolean; | |
} | |
export interface Options extends BaseOptions { | |
// TODO(samstern): Remove this once options is better typed | |
[key: string]: unknown; | |
// whether it's coming from the VS Code Extension | |
isVSCE?: true; | |
} |
Would it be okay to add displayName
as a property in Options
. Something like
Description
Fixes #8634
Scenarios Tested
with [projectId] param
with [display-name] option
Sample Commands
firebase projects:create some-project-id