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

[CLI] Get rid of Yeoman #844

Open
bajtos opened this issue Jan 5, 2018 · 16 comments
Open

[CLI] Get rid of Yeoman #844

bajtos opened this issue Jan 5, 2018 · 16 comments
Assignees
Labels
CLI developer-experience Issues affecting ease of use and overall experience of LB users major refactor

Comments

@bajtos
Copy link
Member

bajtos commented Jan 5, 2018

Refactor the CLI package to use inquirer and ejs directly, get rid of the dependency on Yeoman.

This will allow us to use TypeScript too, we should migrate the CLI codebase to the standard TypeScript project as part of this task.

UPDATE 2020-02-11: Let's not limit ourselves to inquirer and ejs only. The ecosystem offers many more alternatives for building CLI tools, we should research the options and pick the best one. See the comments below for inspiration.

Related:

List of issues compiled from the discussion:

  • In the "old" loopback-cli, our "model" generator is invoking "property" generator in a loop. This is not supported by Yeoman anymore, we have a hacky workaround in place. As a result, upgrades to newer versions of Yeoman were usually painful, because the hacky workaround had to be updated to match the new internals of Yeoman.
  • Invoking a generator directly from CLI is a bit of PITA, there are more hacks needed to make this happen - see e.g. https://github.com/strongloop/loopback-next/blob/6cc83b666e4b2ce2f979eab44dec78b7c01c41be/packages/cli/bin/cli.js#L66-L68
  • I find individual generators difficult to test in isolation. Verifying how CLI arguments are handled is even more difficult.
  • AFAICT, there is no way how to test non-trivial prompting logic - for example the loop asking for multiple properties. A mocked prompt can define only a single set of answers. Running the generator/CLI end-to-end and parsing stdout is brittle because Inquirer uses lots of ANSI control codes to control what's displayed on the screen.
  • Another missing feature is inability to verify configuration of individual prompts. For example when using a choices question populated dynamically (e.g. the datasource prompt showing all datasources known by the app), we want to verify the list of choices offered.
  • I also find the necessity to pepper our generator actions with if (this.shouldExit()) return false; as a horrible code smell. (See e.g. here and here and here). I was took a quick glance at other generators, not all actions start with shouldExit check, so we may already have subtle bugs there!
  • Subjectively, I dislike the conventions forced by Yeoman:
    • All generators live in different index.js files, which makes navigation difficult to me. Instead of app/index.js, I'd like to have app.generator.js file.
    • Implementing individual steps as named methods on a generator object made sense back in ES5 days, because it simplified async flow control. Today with ES2017's async/await, I prefer to write idiomatic JavaScript instead.
  • Reading from stdin has to use readline or complex workarounds, it's not possible to read entire input into a string using the usual streams API.
  • We need to call process.chdir in our tests, which makes it impossible to run the tests in parallel via worker threads. See run Mocha's tests concurrently mochajs/mocha#2839 (We found a solution for this.)
@bajtos bajtos added Core-GA developer-experience Issues affecting ease of use and overall experience of LB users refactor CLI labels Jan 5, 2018
@bajtos bajtos changed the title Get rid of Yeoman [CLI] Get rid of Yeoman Jan 5, 2018
@privateOmega
Copy link

@bajtos I would like to help however I can on this issue.

@bajtos
Copy link
Member Author

bajtos commented Jan 9, 2018

@raymondfeng Here are the issues I was experiencing in the old generator-loopback frequently:

  • Our "model" generator is invoking "property" generator in a loop. This is not supported by Yeoman anymore, we have a hacky workaround in place. As a result, upgrades to newer versions of Yeoman were usually painful, because the hacky workaround had to be updated to match the new internals of Yeoman.
  • Invoking a generator directly from CLI is a bit of PITA, there are more hacks needed to make this happen - see e.g. https://github.com/strongloop/loopback-next/blob/6cc83b666e4b2ce2f979eab44dec78b7c01c41be/packages/cli/bin/cli.js#L66-L68
  • I find individual generators difficult to test in isolation. Verifying how CLI arguments are handled are even more difficult.
  • AFAICT, there is no way how to test non-trivial prompting logic - for example the loop asking for multiple properties. A mocked prompt can define only a single set of answers. Running the generator/CLI end-to-end and parsing stdout is brittle because Inquirer uses lots of ANSI control codes to control what's displayed on the screen.
  • Subjectively, I dislike the conventions forced by Yeoman:
    • All generators live in different index.js files, which makes navigation difficult to me. Instead of app/index.js, I'd like to have app.js file.
    • Implementing individual steps as named methods on a generator object made sense back in ES5 days, because it simplified async flow control. Today with ES2017's async/await, I prefer to write idiomatic JavaScript instead.

@privateOmega thank you for chiming in! Right now, we are debating whether we actually want to drop Yeoman or whether it's perhaps not that bad. Once we reach consensus, I can show you pointers where to start.

@bajtos
Copy link
Member Author

bajtos commented Jan 9, 2018

AFAICT, there is no way how to test non-trivial prompting logic - for example the loop asking for multiple properties. A mocked prompt can define only a single set of answers. Running the generator/CLI end-to-end and parsing stdout is brittle because Inquirer uses lots of ANSI control codes to control what's displayed on the screen.

Another missing feature is inability to verify configuration of individual prompts. For example when using a choices question populated dynamically (e.g. the datasource prompt showing all datasources known by the app), we want to verify the list of choices offered.

@bajtos
Copy link
Member Author

bajtos commented Jan 9, 2018

I also find the necessity to pepper our generator actions with if (this.shouldExit()) return false; as a horrible code smell. (See e.g. here and here and here). I was took a quick glance at other generators, not all actions start with shouldExit check, so we may already have subtle bugs there!

@shimks
Copy link
Contributor

shimks commented Mar 19, 2018

I feel like this is something that should be done ASAP before we pile on more features onto our CLI tool. I'm for replacing Yeoman if we can come up with a prototype that addresses the problems that Yeoman faces.

@raymondfeng
Copy link
Contributor

I suggest that we don't rush into a conclusion. It's probably the best choice to have a time-boxed spike to:

  1. Verify the pain points that @bajtos has listed and see there are fixes/solutions without abandoning Yeoman.

  2. Identify what's needed to build our CLIs using inquirer and ejs directly.

  3. Build a PoC for a simple generator such as app.

@bajtos
Copy link
Member Author

bajtos commented Mar 22, 2018

+1 for starting with a spike.

@virkt25
Copy link
Contributor

virkt25 commented Mar 23, 2018

+1 for spike. Aside from verifying the pain points listed above and the plan outlined by @raymondfeng, are there any additional questions we want to answer in this spike?

@dhmlau
Copy link
Member

dhmlau commented Mar 26, 2018

With the spike #1187 created, I'm not sure what's the best way to handle this issue. Keep it open? change it to be an epic?

@shimks
Copy link
Contributor

shimks commented Aug 1, 2018

An issue that arose from #1578:

  • when error is thrown by throw new Error(), error is both rejected AND emitted. yeoman-test module does not take this into account which leaves us with two rejected promises to deal with.

@bajtos
Copy link
Member Author

bajtos commented Nov 1, 2019

A module we could use for interactive prompts: https://github.com/terkelg/prompts

I especially like autocomplete prompt, I think it will give much better UX for large projects.

@dhmlau dhmlau removed their assignment Dec 16, 2019
@bajtos bajtos added the major label Jan 14, 2020
@bajtos
Copy link
Member Author

bajtos commented Feb 11, 2020

https://oclif.io seems to be a popular library used e.g. by Heroku CLI.

@dougal83
Copy link
Contributor

https://oclif.io seems to be a popular library used e.g. by Heroku CLI.

Looks very good. +1 for plugins @oclif/plugin-not-found (e.g. "did you mean?") @oclif/plugin-autocomplete (add bash autocomplete).

@bajtos
Copy link
Member Author

bajtos commented Feb 28, 2020

Added another pain point:

We need to call process.chdir in our tests, which makes it impossible to run the tests in parallel via worker threads. See mochajs/mocha#2839

@bajtos
Copy link
Member Author

bajtos commented Apr 9, 2020

@mdbetancourt
Copy link
Contributor

@bajtos bajtos removed their assignment Apr 5, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLI developer-experience Issues affecting ease of use and overall experience of LB users major refactor
Projects
None yet
Development

No branches or pull requests

10 participants