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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

[RFC] [WIP] feat(cli-core): add a new package for cli in TypeScript #5062

Closed
wants to merge 4 commits into from

Conversation

raymondfeng
Copy link
Contributor

@raymondfeng raymondfeng commented Apr 8, 2020

This PoC starts to build our CLI commands as TypeScript classes and use @loopback/core for dependency injection and extensibility.

We should allow different CLI commands to be packaged in their own modules and contribute to the core as a component.

Checklist

馃憠 Read and sign the CLA (Contributor License Agreement) 馃憟

  • npm test passes on your machine
  • New tests added or existing tests modified to cover all changes
  • Code conforms with the style guide
  • API Documentation in code was updated
  • Documentation in /docs/site was updated
  • Affected artifact templates in packages/cli were updated
  • Affected example projects in examples/* were updated

馃憠 Check out how to submit a PR 馃憟

@raymondfeng raymondfeng requested a review from bajtos as a code owner April 8, 2020 05:08
@raymondfeng raymondfeng added the CLI label Apr 8, 2020
@raymondfeng raymondfeng force-pushed the cli-core branch 3 times, most recently from 4c6de2e to 2469b6b Compare April 8, 2020 18:15
@bajtos
Copy link
Member

bajtos commented Apr 9, 2020

Assigning CLI owners to review this PR:

@bajtos
Copy link
Member

bajtos commented Apr 9, 2020

We have experienced a ton of issues with Yeoman and would like to move away from it. The issue #844 is keeping track of the problems.

How is this proposal going to affect the process of getting rid of Yeoman?

If we are going to introduce a new architecture for our CLI, can we please design it to be independent and decoupled from Yeoman please?

@bajtos
Copy link
Member

bajtos commented Apr 9, 2020

Thank you @raymondfeng for starting this effort, I agree our CLI codebase is a bit outdated and it's well past time to modernize it.

Please work with @dhmlau to plan the time needed by the team to discuss, asses and review your proposals - both the high-level design/architecture and low-level implementation details. At the moment, the PR is adding more 9.3K lines, that's a lot of code to review!

I think it's important to find a way how to split this work into smaller chunks that are easier to review and can be landed faster. This will reduce merge conflicts with other ongoing work in CLI and make it easier to resolve those conflicts that cannot be avoided. I am proposing to start with a spike, where you build a prototype (a proof of concept) of the target design. Then we can identify small incremental steps for migrating our current CLI version to the new style, preferably one CLI command at a time.


There is one problem I see in the current design that would be great to consider while redesigning our CLI. Consider the following story:

As a LoopBack user, I created a LB4 project back in 2019. Artifact templates used by the CLI have evolved since then, but I didn't have time to upgrade my project yet. To keep thing simple for me, I am keeping my globally installed @loopback/cli at an old version.

Now I am starting a new LB4 project. I would like to use the latest conventions to avoid introducing tech debt. To do that, I need to upgrade my global @loopback/cli to the latest version. However, that will complicate working with the old 2019 project, because the new CLI version is going to create slightly different artifacts compared to what I have already have, introducing inconsistencies.

Ideally, I'd like the CLI tool to understand what version of conventions/templates my project is using and honor that when creating new artifacts.

(end of story)

This has been on my mind for some time, I am thinking about the following solution:

  • The code & templates building artifacts should be a dev-dependency of the LB project.
  • Our CLI should be a thin wrapper invoking the implementation installed inside the project.

I am expecting such design to play very well with lb4 update command, where an update step can modify both dependency versions and the related artifact templates.

I don't have strong opinions on where to draw the line between the parts that should be inside the global CLI and parts that will be installed inside the project. One option is to move all generators to project dev-dependencies and keep only lb4 app in the global CLI. Another option is to keep all CLI interface inside the global CLI package and put only the code & templates building artifacts into the dev-dependency (we can introduce something similar to https://github.com/strongloop/loopback-workspace, see #5000 (comment)).

The transition from keeping all logic in the globally installed tool to moving it to a project dev-dependency is not unique to us, popular tools like mocha and eslint have already gone through it. I remember that at some point, globally-installed mocha used to check if there is a local version of mocha available too and delegated all work to the local one if it was present.

Most recently, I am seeing projects to use even different setup where no global deps are needed.

  • Use npx tool to install & run script to scaffold a new project, e.g. npx create-lb4-app. npx will ensure that the latest version of the script is used, there is no need to maintain an up-to-date version of a global dependency (npm i -g create-lb4-app).
  • Commands for creating in-project artifacts are provided by a dev-dependency.
  • The scaffolded project can provide an npm script to execute those commands from local ./node_modules, e.g. a generic npm run lb4 <command> or a command-specific npm run create-model.
  • Alternatively, we can use npx again to execute code installed locally, e.g. npx lb4 or npx @loopback/create-model.

@@ -0,0 +1,3 @@
# @loopback/cli-core
Copy link
Member

Choose a reason for hiding this comment

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

We should allow different CLI commands to be packaged in their own modules and contribute to the core as a component.

We already have 26 entries under /packages directory, I feel that's already too much.

@loopback/cli has 18 commands (generators) so far. If we put all per-command packages to packages/, we will end up with 44+ entries 馃槺

Let's create a dedicated top-level directory for all new CLI packages please. I am proposing to use the directory name cli, e.g. /cli/cli-core, /cli/model, /cli/repository, etc.

@raymondfeng
Copy link
Contributor Author

@bajtos Thank you for the great feedback. I agree with what you have said.

To clarify, my initial intention is to spend minimal efforts to convert the JS code base to TypeScript and start to embrace our DI. This help us establish an easy-to-refactor baseline.

For broader vision to fully renovate the CLI, a spike will be desired. I'll try to come up a high level markdown document to capture objectives, requirements, and ideas to explore, such as:

High-level requirements:

  1. The new CLI should be extensible and pluggable
  2. The new CLI should be easy to test
  3. The new CLI should support both interactive and headless (for scripting) modes
  4. The new CLI can be used as node modules
  5. Commands should be composable
  6. Error handling - It should be easy to abort/cancel a command

Possible Candidates

  1. Frameworks

  2. Renders

@stale
Copy link

stale bot commented Dec 25, 2020

This pull request has been marked stale because it has not seen activity within two months. It will be closed within 14 days of being stale unless there is new activity.

@stale stale bot added the stale label Dec 25, 2020
@stale
Copy link

stale bot commented Jul 14, 2021

This pull request has been closed due to continued inactivity. If you are interested in finishing the proposed changes, then feel free to re-open this pull request or open a new one.

@stale stale bot closed this Jul 14, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants