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

Fix prompt handler type #617

Merged
merged 1 commit into from
Sep 18, 2023
Merged

Fix prompt handler type #617

merged 1 commit into from
Sep 18, 2023

Conversation

agerard-godaddy
Copy link
Contributor

Summary

More suitable type declaration which is not dependent on deep dependencies.
Fixes error TS2305: Module '"inquirer"' has no exported member 'Inquirer'.

Changelog

@gasket/cli

  • Fix prompt handler type declaration

Test Plan

Tested change in local app.

@agerard-godaddy agerard-godaddy requested a review from a team as a code owner September 18, 2023 18:50
Copy link
Contributor

@mmason2-godaddy mmason2-godaddy left a comment

Choose a reason for hiding this comment

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

👍

@@ -196,7 +195,7 @@ declare module '@gasket/engine' {
prompt(
context: CreateContext,
utils: {
prompt: Inquirer,
prompt: (prompts: Array<Record<string, any>>) => Promise<Record<string,any>>,
Copy link
Contributor

Choose a reason for hiding this comment

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

This doesn't seem like a very helpful type. Can we find a better way to import the type instead?

Copy link
Contributor

@jpage-godaddy jpage-godaddy Sep 18, 2023

Choose a reason for hiding this comment

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

Or add @types/inquirer as a dependency so it is exported? Hmm, looks like maybe there was a version bump and the types don't align now?

Copy link
Contributor

Choose a reason for hiding this comment

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

Looks like @types/inquirer has a PromptFunction type that maybe is what this is?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We set up the prompt function conditionally here. We are using inquirer@6 in the CLI currently, which does not export types. There is a @types/inquirer, but then it becomes a necessary dependency for users to add, unless we make it a direct dependency of the CLI, which doesn't feel right either.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The PromptFunction type is in v9 of @types/inquirer here, but it doesn't exist in v6 version here. Maybe I could use PromptModule from v6? But, either way, this will require a hard dependency of the cli on this @types/inquirer to ensure it is synced up.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We will investigate further when upgrading
#618

@agerard-godaddy agerard-godaddy merged commit 420fe83 into main Sep 18, 2023
3 checks passed
@agerard-godaddy agerard-godaddy deleted the fix-type branch September 18, 2023 20:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants