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

[Brainstorm] Refactoring? #685

Closed
SBoudrias opened this issue May 31, 2018 · 7 comments
Closed

[Brainstorm] Refactoring? #685

SBoudrias opened this issue May 31, 2018 · 7 comments

Comments

@SBoudrias
Copy link
Owner

Discussion triggered by #682, is it time to have a major core refactoring?

A few current pain point:

  1. UT aged really badly
  2. Plugin API isn't great (maybe even horrible?)
  3. Size of some project dependencies are huge

I think going forward with a refactoring we'll want to achieve these goals:

  1. Make it easier to create plugins (I'm probably open to break backward compatibility if the new solution is clearly better)
  2. Maintain high-level API backward compatibility (we don't want users to rewrite their code)
  3. Make prompts work standalone (answer = await password(conf))
  4. Maintains cross-platform compat
  5. Upgrade project tooling to ease onboarding and contribution to the code base
  6. Keep package size in check

I went over the issue backlog, and I don't think there's any major feature request there that would really require major attention during refactoring... Maybe auto-answering, but I'm sure I like this idea so much. Ideas? Opinions?

@SBoudrias
Copy link
Owner Author

cc @dwieeb

@imhoffd
Copy link
Contributor

imhoffd commented May 31, 2018

  • What is UT?
  • Not too familiar with the plugin API, but I tend to be decent at designing public APIs 😊

Among the goals listed, I'd also like to see:

  1. Near immediate startup time. Probably the most important thing. Inquirer is solely used in Node CLI programs, where the runtime is spun up and down frequently. Doing require('inquirier') should be instantaneous. We can accomplish this by dropping unnecessary dependencies and lazy-loading necessary dependencies.
  2. Automatic non-TTY detection and handling. I think this might be the suitable issue for that. I have a few ideas for this, because I've already been wrapping prompt() to provide non-TTY fallbacks.
  3. Easier/better access to the writable stream within Inquirier. By necessity, Inquirer hijacks stdout through readline, but if a CLI writes to stdout while a prompt is showing, the output gets scrambled. I've discovered if I use the BottomBar UI component, it works as expected, but instantiating a BottomBar seems to have its own set of strange issues and it seems kinda hacky. Maybe this code can explain better what seems to be necessary. It'd just be nice to have a more unified, clean interface. I'm not sure what this would look like yet.
  4. A way to have the default value be modified, not just shown and either accepted or rejected.

Lastly, and this might be a touchy subject for some, but I've seen large codebases benefit greatly from using TypeScript. I'd love to see it considered during this proposed refactor.

@SBoudrias
Copy link
Owner Author

Easier/better access to the writable stream

I'm not completely clear on what the issue here is and what is involved. The ionic-cli example seems to just mute the stream to prevent user from typing while the bottom bar is showing up.

@imhoffd
Copy link
Contributor

imhoffd commented Jun 2, 2018

I'll make a simplistic example repo and maybe a video to show what I mean. It's a strange use case of using both prompts and having an instantiated bottom bar at the same time. I think it's a bug.

@wtgtybhertgeghgtwtg
Copy link
Contributor

This'd totally be breaking, but how about removing the Question object and have people pass in a function that can be created by a function?

// Not this
inquirer.prompt([{message: 'Enter your password.', type: 'password'}]);
// This
inquirer.prompt([passwordQuestion({message: 'Enter your password.'})]);

Prompts would no longer have to be registered, meaning that all of the built-in prompts would not have to be require'd at initialization. This might also make custom stuff a little easier, since there's no registration anymore.

@SBoudrias
Copy link
Owner Author

@wtgtybhertgeghgtwtg checkout #688

Went with a similar solution, just no need for inquirer.prompt at all to use prompts if imported directly. We'll keep inquirer.prompt for convenience and backward compatibility, but it won't be a requirement for simpler usage.

@SBoudrias SBoudrias mentioned this issue Jun 23, 2018
29 tasks
@SBoudrias
Copy link
Owner Author

Closing in favor of #692

This issue was closed.
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

No branches or pull requests

3 participants