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

Add support for yarn #171

Merged
merged 2 commits into from Oct 29, 2018
Merged

Conversation

igrayson
Copy link
Contributor

@igrayson igrayson commented Oct 20, 2018

Hello 馃憢. I wasn't sure what the interest level is in this, so I just put together a PR to demonstrate it. My organization uses yarn instead of npm, and this makes concurrently more useful to us.

This adds support for the popular npm replacement, yarn.

ian $ grep watch package.json
    "watch-compile": "node ./scripts/compile-watch.js",
    "watch-generate-client": "node ./scripts/generate-client-watch.js",
    "dev": "yarn compile && concurrently 'yarn compile-watch' 'yarn generate-client-watch'",

ian $ concurrently "yarn:watch-*"
yarn run v1.10.1
yarn run v1.10.1
$ node ./scripts/generate-client-watch.js
$ node ./scripts/compile-watch.js
[watch-generate-client] Watching /Users/ian/workspace/interface/**/*.proto for changes.
[watch-compile] 10:57:39 - Starting compilation in watch mode...
[watch-compile] 10:57:41 - Found 0 errors. Watching for file changes.
[watch-generate-client] Interface file changed: rpc_interface.proto
[watch-generate-client]   Generating: /Users/ian/workspace/client-generated/generated-schema.json
[watch-generate-client]   Generating: /Users/ian/workspace/client-generated/generated-client.js
[watch-generate-client]   Generating: /Users/ian/workspace/client-generated/generated-client.d.ts
[watch-generate-client]   Generating: /Users/ian/workspace/client-generated/package.json
[watch-generate-client]   Generating: /Users/ian/workspace/client-generated/index.ts
[watch-compile] 10:57:44 - File change detected. Starting incremental compilation...
[watch-compile] 10:57:44 - Found 0 errors. Watching for file changes.

@coveralls
Copy link

Pull Request Test Coverage Report for Build 321

  • 2 of 2 (100.0%) changed or added relevant lines in 2 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage remained the same at 99.178%

Totals Coverage Status
Change from base Build 320: 0.0%
Covered Lines: 239
Relevant Lines: 239

馃挍 - Coveralls

@coveralls
Copy link

coveralls commented Oct 20, 2018

Pull Request Test Coverage Report for Build 324

  • 2 of 2 (100.0%) changed or added relevant lines in 2 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage remained the same at 99.178%

Totals Coverage Status
Change from base Build 320: 0.0%
Covered Lines: 239
Relevant Lines: 239

馃挍 - Coveralls

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.

Thanks for implementing this!
I had in mind that someone would like to have yarn support very soon when rewriting concurrently for the v4 release...

You took a good approach here.
However I feel we need to adjust something: yarn run <script> -- <args> isn't exactly the optimal way to call a yarn script.

It yields this for me:

$ yarn test:watch -- home
yarn run v1.7.0
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.

I think there should be some per-tool customisation in the command building.
The other option would be to split the implementations.

What do you think?

README.md Show resolved Hide resolved
@igrayson
Copy link
Contributor Author

igrayson commented Oct 23, 2018

I may not fully understand the concern. When I look at how our tool builds commands, I don't see a scenario where we add --s. To my understanding, concurrently would only execute yarn test:watch -- home if the input from the user included the double-dash.

E.g. concurrently "yarn:test:* -- home" -> yarn test:watch -- home, and
concurrently "yarn:test:* home" -> yarn test:watch home. To me, this behavior seems like what we want?

@gustavohenke
Copy link
Member

Oh, sorry. You are correct.
I probably got this impression from the tests, as they are using npm-tailored commands (with --). Therefore your improvement looks good to me :)

@gustavohenke gustavohenke self-requested a review October 25, 2018 00:47
@gustavohenke gustavohenke merged commit 3173b36 into open-cli-tools:master Oct 29, 2018
@gustavohenke
Copy link
Member

Published on v4.1.0! 馃殺

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants