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

[rush] rushx should support parameters for commands #1232

Closed
Toxaris opened this issue Apr 14, 2019 · 24 comments · Fixed by #2096
Closed

[rush] rushx should support parameters for commands #1232

Toxaris opened this issue Apr 14, 2019 · 24 comments · Fixed by #2096
Labels
effort: medium Needs a somewhat experienced developer enhancement The issue is asking for a new feature or design change help wanted If you're looking to contribute, this issue is a good place to start! needs design The next step is for someone to propose the details of an approach for solving the problem

Comments

@Toxaris
Copy link

Toxaris commented Apr 14, 2019

We want to use rushx as a drop-in replacement for npm run but we are missing the ability to pass through the remainder of the command line to the invoked script. For example, we would like to call rushx foo bar baz and it should run the script named foo with the command line bar baz.

@octogonz octogonz added the enhancement The issue is asking for a new feature or design change label May 29, 2019
@octogonz octogonz changed the title rushx should pass command line to invoked script [rush] rushx should pass command line to invoked script May 29, 2019
@octogonz
Copy link
Collaborator

@Toraxis sorry nobody replied until now.

For a little background, there's a couple reasons why this wasn't implemented originally:

  1. The Rush philosophy is that any time you introduce a command line to be used by hundreds of developers in a monorepo, it must have complete command-line help (e.g. what are the supported parameters? what does each one do?) and validation (i.e. don't blindly perform an operation if I misspelled the parameter name). Otherwise the command can be difficult to support. The Rush commands follow this same philosophy. I had sketched out a design for defining custom rushx commands, but unfortunately this feature got tabled because nobody was asking for it at the time. We could definitely bring it back.

  2. Rush parses its command line using the ts-command-line library, which implements this philosophy. So if I remember right there might be some parser work required to pass along "whatever else you put on the command line." (?)

If I was going to work on this, personally I'd invest my time in the documented/validated design rather than the npm run design. However if somebody wants to contribute a PR implementing the npm run design, we'd accept it, perhaps as a default behavior in the absence of any other configuration. It's certainly practical if not ideal.

@octogonz octogonz added effort: medium Needs a somewhat experienced developer help wanted If you're looking to contribute, this issue is a good place to start! needs design The next step is for someone to propose the details of an approach for solving the problem labels May 29, 2019
@octogonz octogonz changed the title [rush] rushx should pass command line to invoked script [rush] rushx should support parameters for commands May 29, 2019
@poelstra
Copy link

It's sometimes handy to just pass some ad-hoc arguments to existing scripts (e.g. some extra arguments to a test runner while debugging).

it must have complete command-line help

This certainly makes sense.

Note that npm run doesn't simply pass unknown stuff to a script, only arguments coming after --, e.g. rushx foo -- bar baz.

Hopefully that could help with keeping the command parser both 'strict' for normal cases, yet also flexible for these cases?

@domarmstrong
Copy link
Contributor

I'm also running into this but with command-line scripts. I have my own cli for various tasks, but it would be pretty handy to be able to use for example rush {task} blah --stuff. I am using yargs myself so arguments are already being validated, and in some cases positional arguments are used (which I dont think you support).

eg rush create library my-new-lib > tools/cli/bin/cli create library my-new-lib

It would be a great benefit to be able to use rush as the command runner.

It would be nice to have an additional option on the command something like skipArgumentValidation: boolean

    {
      "commandKind": "global",
      "name": "create",
      "summary": "Create new app/component/library",
      "description": "Create new app/component/library using tools/cli",
      "safeForSimultaneousRushProcesses": false,
      "shellCommand": "node ./tools/cli/bin/cli.js create",
      "skipArgumentValidation": true
    },

@octogonz
Copy link
Collaborator

octogonz commented Jun 2, 2019

That could work. How would we ensure there is command-line help for such a command? What is the user experience like if someone misspells a parameter name?

@domarmstrong
Copy link
Contributor

Using yargs I'm getting the following. But there are times I can see you may just want to basically expose another cli and you would have to rely on their own validation. Copying all the options over to the command doesn't sound great, and then in my current use case of positional arguments rush doesn't support.

$ rush create


Rush Multi-Project Build Tool 5.7.1 - https://rushjs.io


Starting "rush create"

Error:

>  Not enough non-option arguments: got 0, need at least 2 

rush create <type> <name>

create new app/feature/library/tool/component

all-packages
  --type, -t  (package.json) package type
            [string] [choices: "app", "feature", "library", "tool", "component"]

apps
  --title    (package.json) browser tab name                            [string]
  --baseUrl  (package.json) applications base url                       [string]

Options:
  --version  Show version number                                       [boolean]
  --help     Show help                                                 [boolean]

The script failed with exit code 1

@ssalbdivad
Copy link

@octogonz Would love to see this implemented. I'd grown accustomed to always using rushx over npm run, but often I often find it useful to pass process args via the command line as opposed to modifying my package.json just to get debug output, for example.

@octogonz
Copy link
Collaborator

Maybe rushx could support two models:

  • Rush model: We'll introduce a config file that allows individual projects to declare their command-line for rushx. It would be similar to how command-line.json works for bulk commands, except we would also support an "$extends" field that enables standard commands to be defined by a shared toolchain package.
  • NPM compatible model: When this config file is NOT present for a project, then rushx falls back to passing its command-line along to the script just like npm run.

This way the more structured model is opt-in (and we can implement what you're asking for without waiting for the structured model to be designed/implemented).

@ssalbdivad
Copy link

@octogonz That would be great :bowtie: Best of both worlds!

@devin-fee-ah
Copy link

@octogonz what's the best way to move this forward?

Based on a conversation in the gitter chat... I believe people are taking the approach of having a package.json in the root of the project – which is definitely not ideal.

@dfee
Copy link

dfee commented May 7, 2020

I also believe there is the need for a global command support. i.e. I want to expose a scaffold generator: rush generate --project foo. This doesn't seem to be reflected in #1232 (comment)

@octogonz
Copy link
Collaborator

octogonz commented May 8, 2020

I also believe there is the need for a global command support.

@dfee Global commands are already supported using rush instead of rushx. See command-line.json docs. If I'm misunderstanding something, could you give more detail about this idea?

@octogonz
Copy link
Collaborator

octogonz commented May 8, 2020

@octogonz what's the best way to move this forward?

@devin-fee-ah the "needs design" issue tag is referring to the Rush model I outlined above. We need someone to propose how that would work.

For the NPM compatible model, someone just needs to make a PR. It's probably a relatively easy work item. If someone wants to work on it and needs help, feel free to ask in Gitter. Personally I've been pretty swamped recently, but if you ping me directly on Gitter I usually respond fairly quickly.

@dfee
Copy link

dfee commented May 12, 2020

Hopefully I can clarify the issue that I'm facing. To use the words of @octogonz as a template:

What I'm looking for is this: for a particular command, where the underlying script already performs:

  1. CLI validation and,
  2. --help docs,

That I could simply pass-through Rush's command line arguments without checking them.

The most trivial example, useful for communicating the feature:

{
  "commandKind": "global",
  "name": "echo",
  "summary": "passes args to echo",
  "shellCommand": "echo"
},

Then: rush echo hi would pipe hi to stdout.

The general use-case (for me) is where I have tools (e.g. scaffolding new projects) that are already built with ts-command-line. It doesn't make sense for me to define that command, and its options / arguments in two separate places (and to try and keep them in sync).

@dfee
Copy link

dfee commented May 12, 2020

Also, let me be clear - I'm totally willing to attempt a fix at this... but will likely need some shepherding.

I wonder if the approach would be creating an override here: https://github.com/microsoft/rushstack/blob/bf36e86d65b5c1dbf083221d13594e88123b081e/libraries/ts-command-line/src/CommandLineAction.ts

@octogonz
Copy link
Collaborator

You're right that this needs to start with a ts-command-line feature, which will raise an API design question. Lemme see if I can come up with something for that part.

@octogonz
Copy link
Collaborator

I made some progress on this. Will aim to put up a PR soon.

@octogonz
Copy link
Collaborator

Here's a PR: #1869

It was more work than I thought. :-) Mostly remembering how to use argparse and figuring out an API that didn't conflict with other features.

@octogonz
Copy link
Collaborator

octogonz commented May 15, 2020

I merged the PR. Publishing failed because npm, Inc. seems to have a major outage, but that shouldn't block the next step for rushx.

Edit: We successfully published @rushstack/ts-command-line 4.4.0.

@dfee
Copy link

dfee commented May 18, 2020

So it looks like the next steps are:

  1. push an update to ts-command-line api
  2. add support into rush and update the command-line.schema.json for rush commmands
  3. figure out how to add this for rushx commands

@octogonz
Copy link
Collaborator

I'll take care of #1. If you have questions about the other steps, I'm also reachable on Gitter.

@octogonz
Copy link
Collaborator

This should take care of #1: microsoft/rushstack.io-website#15

Seems like the API docs haven't been updated in a while. :-)

@octogonz
Copy link
Collaborator

Here's a PR that implement's @Toxaris's original suggestion: #2096

It turned out to be easier than expected, since rushx doesn't even use ts-command-line.

@allbto
Copy link

allbto commented Feb 3, 2021

So it looks like the next steps are:

  1. push an update to ts-command-line api
  2. add support into rush and update the command-line.schema.json for rush commmands
  3. figure out how to add this for rushx commands

@octogonz It seems like 2. was not addressed yet to allow rush command-line.json to use the newly implemented CommandLineRemainder feature from #1869

Any plans to add this? It would greatly improve the rush experience IMO

@octogonz
Copy link
Collaborator

@allbto After CommandLineRemainder was released, I encountered some problems with. Thanks for reminding me about this -- I finally got around to creating an issue to get it fixed: #2499

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
effort: medium Needs a somewhat experienced developer enhancement The issue is asking for a new feature or design change help wanted If you're looking to contribute, this issue is a good place to start! needs design The next step is for someone to propose the details of an approach for solving the problem
Projects
None yet
8 participants