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

Support passthrough of additional arguments to commands via placeholders #307

Merged
merged 15 commits into from May 9, 2022
Merged

Support passthrough of additional arguments to commands via placeholders #307

merged 15 commits into from May 9, 2022

Conversation

paescuj
Copy link
Collaborator

@paescuj paescuj commented Feb 15, 2022

This pull request adds support to passthrough additional / positional arguments (passed to concurrently via command line after --) to individual commands via the following placeholders:

  • {<number>} (for example {1}) to use a single argument at a specific position
  • {@} to use all arguments
  • {*} to use all arguments combined into one argument

This feature is disabled by default (so no breaking change) and can be enabled via the --passthrough-arguments (alias -P) option.

For example:

concurrently -P "echo {1}" -- foo bar
# Results in: echo foo

concurrently -P "npm:dev-* -- {@}" -- -w --noEmit
# Results in something like: npm run dev-example -- -w --noEmit

concurrently -P "echo {*}" -- foo bar
# Results in: echo 'foo bar'

This is especially useful if you want to run essentially the same scripts while one of it having additional arguments (build vs. dev):

"scripts": {
    "build": "concurrently -P 'npm:build:* -- {@}' --",
    "build:esm": "tsc --project ./tsconfig.json --module ES2015 --outDir ./dist/esm",
    "build:cjs": "tsc --project ./tsconfig.json --module CommonJS --outDir ./dist/cjs",
    "dev": "npm run build -- -w --preserveWatchOutput --incremental"
}

I decided to use placeholders because I think this gives us the most flexible solution. For example, this allows us to pass the arguments to only some of the commands instead of all commands:

concurrently -P \
  "echo 'Passed to this command: {1}'" \
  "echo 'Not passed to this command'" \
  -- foo

Inspired by npm-run-all and #282 (Thanks!).

Tests are included in this pull request. Additionally, I've tested some scenarios manually (like the one above with package.json scripts).

Closes #33
Closes #282 (as replacement)


In addition this pull requests also contains a couple of small enhancements, some of them in separate commits:

  • Add missing comment for 'prefixColor' - 7e98293
  • Replace deprecated 'substr' method by 'substring' - 063bde7
  • Fix & enhance EditorConfig - b9b27e8

@coveralls
Copy link

coveralls commented Feb 15, 2022

Coverage Status

Coverage increased (+0.1%) to 97.809% when pulling a370af3 on paescuj:argument-passthrough into 39fddef on open-cli-tools:master.

@paescuj paescuj marked this pull request as ready for review February 15, 2022 12:09
@paescuj
Copy link
Collaborator Author

paescuj commented Mar 9, 2022

Hi, @gustavohenke! This PR is now rebased on the latest changes. After thinking about it, I've decided to introduce a new option to toggle this feature. The option is disabled by default which means this PR is not a breaking change. Hopefully, this makes it easier for you to consider this PR "acceptable". Thanks!

@gustavohenke gustavohenke self-requested a review March 19, 2022 09:41
package.json Show resolved Hide resolved
.eslintrc.json Outdated Show resolved Hide resolved
src/command-parser/expand-arguments.ts Outdated Show resolved Hide resolved
src/command-parser/expand-arguments.ts Outdated Show resolved Hide resolved
src/command-parser/expand-arguments.ts Outdated Show resolved Hide resolved
src/concurrently.ts Outdated Show resolved Hide resolved
@gustavohenke gustavohenke self-requested a review April 23, 2022 08:36
@paescuj
Copy link
Collaborator Author

paescuj commented Apr 25, 2022

Thanks @gustavohenke for your review! The PR has been updated accordingly.

Copy link
Member

@gustavohenke gustavohenke left a comment

Choose a reason for hiding this comment

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

Awesome job! Only a couple more suggestions and it should be good to go.

src/command-parser/expand-arguments.ts Outdated Show resolved Hide resolved
src/concurrently.ts Outdated Show resolved Hide resolved
src/concurrently.ts Outdated Show resolved Hide resolved
src/concurrently.spec.ts Show resolved Hide resolved
src/command-parser/expand-arguments.ts Outdated Show resolved Hide resolved
src/command-parser/expand-arguments.ts Outdated Show resolved Hide resolved
src/command-parser/expand-arguments.spec.ts Outdated Show resolved Hide resolved
@paescuj paescuj requested a review from gustavohenke May 2, 2022 10:16
Copy link
Member

@gustavohenke gustavohenke left a comment

Choose a reason for hiding this comment

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

Awesome job, and welcome aboard! Feel free to merge when ready 😃

@paescuj
Copy link
Collaborator Author

paescuj commented May 9, 2022

Awesome job, and welcome aboard! Feel free to merge when ready 😃

Thank you very much! 😃

@paescuj paescuj merged commit e9d2b94 into open-cli-tools:master May 9, 2022
@paescuj paescuj deleted the argument-passthrough branch May 9, 2022 09:29
@ezmiller

This comment was marked as outdated.

@gustavohenke

This comment was marked as outdated.

@ezmiller

This comment was marked as outdated.

@paescuj

This comment was marked as outdated.

@gustavohenke
Copy link
Member

Alright this is now out in v7.2.0! 🚢

@ezmiller

This comment was marked as duplicate.

@ezmiller
Copy link

Maybe it's a problem with use with yarn specifically?

~/Projects/test-conc > npx concurrently -P "echo {1}" -- foo                                                                                        emiller at C02Z21Z9LVDL

[0] foo
[0] echo foo exited with code 0
~/Projects/test-conc > yarn concurrently -P "echo {1}" -- foo                                                                                       emiller at C02Z21Z9LVDL
yarn run v1.22.5
warning From Yarn 1.0 onwards, scripts don't require "--" for options to be forwarded. In a future version, any explicit "--" will be forwarded as-is to the scripts.
$ /Users/emiller/Projects/test-conc/node_modules/.bin/concurrently foo
[0] /bin/sh: foo: command not found
[0] foo exited with code 127
error Command failed with exit code 1.
info Visit https://yarnpkg.com/en/docs/cli/run for documentation about this command.

@paescuj
Copy link
Collaborator Author

paescuj commented May 16, 2022

Maybe it's a problem with use with yarn specifically?

Yes, this is specific to yarn. As you can see yarn (unfortunately?) treats the "--" differently and only passes the arguments after it to concurrently:

yarn concurrently -P "echo {1}" -- foo
yarn run v1.22.18
warning From Yarn 1.0 onwards, scripts don't require "--" for options to be forwarded. In a future version, any explicit "--" will be forwarded as-is to the scripts.
$ /[...]/node_modules/.bin/concurrently foo

On the command line you can work around this by adding an additional "--" in front of the actual arguments:

yarn concurrently -- -P "echo {1}" -- foo

When using concurrently in the scripts section of package.json, however, it seems to work as intended (no workaround required):

  "scripts": {
    "example": "concurrently -P 'echo {1}' -- foo"
  },
yarn example
yarn run v1.22.18
$ concurrently -P 'echo {1}' -- foo
[0] foo
[0] echo foo exited with code 0

@Nantris
Copy link

Nantris commented May 26, 2022

Args we pass end up with unexpected quotation marks around them, and slashes.

We're trying to pass args to Webpack like this:

"build-dev": "concurrently --passthrough-arguments \"npm run build-preload -- {@}\" \"npm run build-main-dev -- {@}\" \"npm run build-renderer-dev -- {@}\" -- ",
"package-demo": "npm run build-dev -- --env IS_DEMO_MODE=true",

When we run yarn package-demo and then console.log(env) in our Webpack config, it looks like this:

{
 WEBPACK_BUNDLE: true,
 WEBPACK_BUILD: true,
 'IS_DEMO_MODE\\\\': 'true'
}
  1. The run command ends up looking like this, with unexpected quotes:

concurrently --passthrough-arguments "npm run build-preload -- {@}" "npm run build-main-prod -- {@}" "npm run build-renderer-prod -- {@}" -- "--env" "IS_DEMO_MODE=true"

  1. If I run it straight from the command line like this, then the args passed through to Webpack still have unexpected slashes and quotes, but fewer slashes:

concurrently --passthrough-arguments "npm run build-preload -- {@}" "npm run build-main-dev -- {@}" "npm run build-renderer-dev -- {@}" -- --env IS_DEMO_MODE=true

{ 
  WEBPACK_BUNDLE: true,
  WEBPACK_BUILD: true,
  'IS_DEMO_MODE\\': 'true'
}

We also tried using --raw with no change in behavior. Any thoughts?

Big thanks to @paescuj for working on this!


Edit:

If we use --env IS_DEMO_MODE instead of --env IS_DEMO_MODE=true it seems to work as intended, I think. I need to do some further testing.

@paescuj
Copy link
Collaborator Author

paescuj commented May 31, 2022

Hey @slapbox, thanks for your feedback!
I guess you're using Windows, right? Which shell? I cannot reproduce this on MacOS with bash.
Can you also share one of the webpack (e.g. build-preload) scripts with me? Thanks!

@Nantris
Copy link

Nantris commented Jun 1, 2022

Hey @paescuj. Indeed it is Windows, and a bit strange because we're using zsh through cygwin.

The script build-preload is a simple command that looks like this: webpack --config ./webpack_config/webpack.config.preload.babel.js

@paescuj
Copy link
Collaborator Author

paescuj commented Jul 21, 2022

Hey @paescuj. Indeed it is Windows, and a bit strange because we're using zsh through cygwin.

The script build-preload is a simple command that looks like this: webpack --config ./webpack_config/webpack.config.preload.babel.js

Hey @slapbox, just wanna let you know that this issues is still on my todo list... I have some ideas in my head to get rid of it, but I need some time for testing & implementation.

@Nantris
Copy link

Nantris commented Jul 21, 2022

@paescuj thanks for the follow-up! I was able to get it working in our case by using DEMO_MODE rather than DEMO_MODE=true, but for cases where the value isn't a boolean I guess that's blocked for now.

"build-dev": "concurrently --passthrough-arguments \"npm run build-preload -- {@}\" \"npm run build-main -- {@}\" \"npm run build-renderer -- {@}\" -- ",
"package-demo": "npm run build-dev -- --env IS_DEMO_MODE",

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.

Add parameter/argument passthrough option
5 participants