Refactor Inquirer to new package structure

[martinlingstuyl]

Inquirer has been completely rewritten in recent times. What we have been installing is the old version, which is still maintained.

This is what the maintainer said about it:

Inquirer recently underwent a rewrite from the ground up to reduce the package size and improve performance. The previous version of the package is still maintained (though not actively developed), and offered hundreds of community contributed prompts that might not have been migrated to the latest API. If this is what you're looking for, the previous package is over here.

Source

I've been refactoring it and it turned out to be quite some work, which also led to a couple of questions, because some features have been dropped in the new packages.

This issue is for discussing the implementation.

Implementation

do check out my associated PR #5511 which has exactly what I suggest below here.

Packages
We swap the packages inquirer and @types/inquirer with @inquirer/input, @inquirer/confirm and @inquirer/select.

Prompt util
We create a prompt util in utils/prompt.ts to wrap interactions with inquirer, because inquirer is ESM and cannot be stubbed. We create three separate prompt functions in there: prompt.forConfirmation, prompt.forInput and prompt.forSelection. These functions have a single parameter for passing prompt configuration. (typed the same as what inquirer expects)

We import the correct inquire package (@inquirer/input, @inquirer/confirm, @inquirer/select) when it is needed and pass the prompt config parameter into the inquire prompt call.

Cli runtime
We create two static prompt functions in the CLI runtime class, which can be used to toggle the spinner and run the prompts. Cli.promptForSelection and Cli.promptForConfirmation, just like currently with Cli.prompt. Cli.promptForInput is currently not necessary as there is no occasion in the code base where this would be used.

...and of course: replace all instances of where inquirer is used with the new prompt util and all instances where Cli.prompt was used with the new static functions.

[martinlingstuyl]

Any thoughts on this @pnp/cli-for-microsoft-365-maintainers?

[waldekmastykarz]

Why not keep the current Cli.prompt method and abstract all inquirer intricacies away without having to change our code?

[martinlingstuyl]

Well, that is a possibility. I see a couple of downsides @waldekmastykarz:

Some functionality has been dropped from inquirer, like using either 1 or multiple prompts, optionally linking them through the when property.
Also in the old way you can pass in questions and answers as separate objects, but in the new structure this is just one object. One prompt, with a default value.

The structure has changed significantly.

If we would work around that we would have to do quite some coding to do it right and to connect those data structures. While we also would be missing out on working with inquirer prompts in a typed way.

I'd say it's better to change it up. Even if its a lot of work.

[martinlingstuyl]

And also I think the new prompt functions are simpler and therefore more elegant and straightforward.

[martinlingstuyl]

To ask a question in return: why would we build an abstraction layer?
The only reasons that come to mind:

• to save time refactoring
• to decouple more from inquirer

Personally I'd say the first would be a bad reason and the second can be done anyway.

Also I'd dislike if we had to build a lot of abstraction to make sure we can accommodate a 'list of multiple questions' that is currently only used in the m365 setup command. Id say: leave the implementation of how many questions and what order etc in the command itself, let's keep the functions simple and elegant:

Are we requesting a Boolean? Let's prompt for one. Are we requesting input: let's prompt for that.

An added benefit is dat stubbing prompts in tests are nicely separated as well.

Curious te hear your response

[martinlingstuyl]

Any feedback on this one @pnp/cli-for-microsoft-365-maintainers ?

[waldekmastykarz]

Good arguments @martinlingstuyl. +1 to refactor as you proposed

[martinlingstuyl]

Followup question @waldekmastykarz, @pnp/cli-for-microsoft-365-maintainers : what do you think is better:

// 1. prompt options in the form of an object that can be passed into inquirer 1 on 1.
prompt.forConfirmatiom(promptOptions:Options)
// Pro's: no abstraction between what inquirer needs and we give
// Cons: less simple and elegant

// 2. prompt options in the form of (only the necessary) parameters
prompt.forConfirmatiom(message: string, defaultValue: boolean)
// pro: elegant and simple
// con: extra inquirer options not easily added

// 3. do both through overloads, so you can use what you need.

[martinlingstuyl]

Ok @pnp/cli-for-microsoft-365-maintainers, I've set this to 'Work in progress' as I've already created the PR for this.

[waldekmastykarz]

Followup question @waldekmastykarz, @pnp/cli-for-microsoft-365-maintainers : what do you think is better:

// 1. prompt options in the form of an object that can be passed into inquirer 1 on 1.
prompt.forConfirmatiom(promptOptions:Options)
// Pro's: no abstraction between what inquirer needs and we give
// Cons: less simple and elegant

// 2. prompt options in the form of (only the necessary) parameters
prompt.forConfirmatiom(message: string, defaultValue: boolean)
// pro: elegant and simple
// con: extra inquirer options not easily added

// 3. do both through overloads, so you can use what you need.

Option #1: it's clearer and more flexible for the future because it allows us to introduce new property without changing the signature. In option #2, the callers can quickly lose clarity with multiple params of the same type: prompt.forConfirmation(what's this value for?, and this one?, true, false)

[martinlingstuyl]

@waldekmastykarz: fortunately that's what I already implemented 😂