microsoft / rushstack

Monorepo for tools developed by the Rush Stack community
https://rushstack.io/
Other
5.93k stars 595 forks source link

[rush] `rush add` improvements #2889

Open Faithfinder opened 3 years ago

Faithfinder commented 3 years ago

Summary

There are two things I've found weird/inconvenient about rush add.

Extra parameter

The need for -p parameter. All the package managers accept packages to install directly after the command (i.e. yarn add, npm install, pnpm add). This means that developers new to rush need to switch their mindsets when they start working with it. It's not a high barrier, but I don't see a good reason for it to exist at all. From what I gather the underlying library @rushstack/ts-command-line doesn't support params like this? Meaning that would have to be updated first.

No multi-packages

Again, all the major package managers allow you to specify as many packages as you want when you're installing. rush add doesn't and I don't see why. (I checked and it seems multiple -p params have a small gotcha - only the latest is taken into account instead of throwing or allowing multiples. Also something to be added to @rushstack/ts-command-line?)

Question Answer
Would you consider contributing a PR? Yes

Both of those are really low priority in the grand scheme of things, so I thought I could at my leisure use those issues to try contributing. If I don't find the energy - nobody will be really disappointed. The issue is open to get a blessing to start working on this or have a discussion in case some of it is controversial.

Faithfinder commented 3 years ago

Seems like there are some open PR's that get the command closer to what I'd like?

2459

2784

They're not enough, and I'm somewhat worried that #2784 might clash with what I thought would be the implementation for dropping -p.

elliot-nelson commented 3 years ago

When it comes to having "rush add" add multiple packages, there's two changes to the codebase to make: first, the logic of the Add command itself needs to support it, and second, the CLI itself needs to have a way to pass the desired packages to the command.

PR #2459 is helpful because it takes care of the first part, and implements the second given the tools we have today. Maybe we can try to dust that off and get it merged this week.

The other part might need to wait a little longer, a larger rewrite of the TS-command-line is coming up that should allow us to bring the "rush add" CLI closer in alignment with other package managers.

iclanton commented 3 years ago

This would both be great ergonomics improvements IMO. Supporting multiple -p parameters should be straightforward enough. I'll try to take a look at that PR today.

Dropping the need for the explicit "-p" should be supported by the CLI parsing library that we're using, but the TS API to expose that may be tricky. I'd be interested to see if anyone has any suggestions.

Faithfinder commented 3 years ago

The way I thought to implement that would be to add an implicit boolean property to the IBaseCommandLineDefinition.

It could be specified on only one parameter (don't see ways to enforce this except to check and throw at runtime?). If it's specified than any "floating" values would be parsed as this parameter.

This would mean that if "-p" is an implicit parameter this is parsed as both packages added to devDependencies: rush add package1 --dev package2.

This does conflict with #2784 - tool action implicitValue1 --arrayParam arrayValue1 ambiguousValue becomes ambiguous

chengcyber commented 3 years ago

Technically, I did some experiements and concludes to the culprit is argparse package. If given rush add react --all, argparse see the rest parameters is ['react', '--all'], instead of ['react'] and a flag parameter all. You must call with rush add --all react closely to get the wanted result.

I have seen complains to the inconvenience of rush app -p serveral times...Is now the right time to consider switching to another options parser library?

Candidates:

Workaround: one of my colleague wrote a zsh script to handle this issue

function rap() {
    if [ $# -eq 0 ]; then
        echo "No arguments supplied"
        return -1
    fi
    while [ $# -ne 1 ]; do
        rush add -p $1 -s
        shift
    done
    rush add -p $1
}
function rapd() {
    if [ $# -eq 0 ]; then
        echo "No arguments supplied"
        return -1
    fi
    while [ $# -ne 1 ]; do
        rush add -p $1 --dev -s
        shift
    done
    rush add -p $1 --dev
}