remove parallel #94

kentcdodds · 2017-02-06T18:27:46Z

What: Removes parallel

Why: Reduces complexity of the tool and its implementation.

How: It's still a work in progres...

This will be a breaking change... Still evaluating.

tleunen · 2017-02-06T18:48:02Z

😢

Why removing it? Would be great to keep it but use concurrently under the hood instead?

kentcdodds · 2017-02-06T19:02:21Z

I'm looking the reduced complexity from both an API standpoint as well as an implementation standpoint. It's altogether simpler and more focused. And we get even more features and functionality by deferring to another package for this. Still considering it.

kentcdodds · 2017-02-07T16:12:37Z

I'd like to put this to a vote. For clarity, here's what's happening here:

This removes the --parallel feature from p-s and instead recommends usage of concurrently to have scripts that run at the same time.

Arguments for:

Reduces the tool's API surface area. Less for people to learn about the tool.
Simplifies the implementation (considerably) and pushes things off to another tool which can have more uses and is well tested.
Follows the DOTADIW philosophy. Allows composability of existing tools that each "do one thing and do it well" which can be really nice and flexible.

Arguments against:

Users have to install another dependency to get the parallel functionality.

Please use GitHub emoji reactions on this comment to vote.

👍 means "Yes, let's remove --parallel in favor of recommending concurrently"
👎 means "No, let's not remove --parallel"
😕 means "I'm unsure"
😄 means "I don't really care either way"

I'll check back in ~24 hours. I'll try to reach out to as many people as use the tool as possible. Here's a start:

cc/ @DavidWells, @abhishekisnot, @rowanoulton, @giladgo, @tim-mcgee, @nkbt, @tleunen, @Hypercubed, @jisaacks, @boneskull, @RobinMalfait, @edm00se, @SamVerschueren, @sxn, @gunnx, @martellaj, @msegado, @beeman, @elijahmanor

Feel free to comment if you have additional input/thoughts.

sxn · 2017-02-07T16:32:04Z

src/index.js

-  } else {
-    return runSeries()
-  }
+  return runSeries()


if we do end up removing it we should probably open a refactoring issue to clean this type of things up :)

sxn · 2017-02-07T16:38:47Z

README.md

@@ -75,7 +77,7 @@ scripts:
    build:
        default: webpack
        prod: webpack -p
-    validate: nps --parallel lint,test,build
+    validate: concurrently "nps lint" "nps test" "nps build"


Concurrently might prove an extra headache for Cygwin users, I guess. I'll try to see if there's any way to fix that though instead of using that as a basis for the decision at hand.

Thanks for finding that. Seems like it wouldn't be too terrible to fix.

sxn · 2017-02-07T16:43:22Z

I'd go with a 😕 for now. Reducing the surface area's great (as well as having less code to maintain :D) but I'm wondering if in the long run it won't complicate things for users more than it simplifies them.

tleunen · 2017-02-07T16:43:49Z

README.md

      default: 'webpack',
      prod: 'webpack -p',
    },
-    validate: 'nps --parallel lint,test,build',
+    // learn more about concurrently here: https://npm.im/concurrently
+    validate: 'concurrently "nps lint" "nps test" "nps build"',


This is quite verbose.
If this is not possible yet, could we support this syntax?

concurrently "nps lint test build"

@tleunen the issue is that we now have to rely on how concurrently works, and it expects complete commands, not just the parameters like you would with nps -p

I don't think that we could support that syntax unfortunately. Because concurrently will spawn a process with the command nps lint test build. If you look at the additions in EXAMPLES.md you'll see that a function can be used to simplify things further. Perhaps we could use that example in the README instead?

You can also check out the changes to package-scripts.js in this PR. I think it's pretty clean.

gunnx · 2017-02-07T16:51:35Z

Any reason for concurrently over npm-run-all?

…

On 7 Feb 2017 16:45, "Kent C. Dodds" ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In README.md <#94>: > default: 'webpack', prod: 'webpack -p', }, - validate: 'nps --parallel lint,test,build', + // learn more about concurrently here: https://npm.im/concurrently + validate: 'concurrently "nps lint" "nps test" "nps build"', I don't think that we could support that syntax unfortunately. Because concurrently will spawn a process with the command nps lint test build. If you look at the additions in EXAMPLES.md you'll see that a function can be used to simplify things further. Perhaps we could use that example in the README instead? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#94>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AB4Pj7l2d3Hpxa7gw--y9lBimmZ6UG4gks5raJ-8gaJpZM4L4jPv> .

kentcdodds · 2017-02-07T16:56:32Z

Any reason for concurrently over npm-run-all?

Unless I'm mistaken, npm-run-all will only run npm scripts. Because we don't actually put our scripts in package.json it's incompatibile.

When I use raw npm scripts, npm-run-all is one of my favorite packages :)

kentcdodds · 2017-02-07T17:09:39Z

Sorry everyone, I had a copy/paste issue. I'm pretty sure the intent is still the same, but I updated the emoji voting option descriptions to make sure things were more clear.

kentcdodds · 2017-02-07T18:16:59Z

Just updated this PR. Here's a little backstory:

concurrency has this cool feature where it will allow you to label the output from each script (because normally the output is jumbled together and you have to kinda guess where the output is coming from).
However, enabling this functionality caused the colors from the output to be removed :-( This made the output even harder to read than before.
concurrency allows you to pass --raw and we're basically back to where we were with the --parallel flag where the output of the scripts is jumbled together. Status quo.
I dug into it a bit and found out that colors (the module we were using to color our output) wont color the output if it detects that you're in an environment that doesn't support color.
I dug more and found out that chalk (the de-facto standard for coloring output for the terminal these days) also attempts to detect whether to actually color the strings you give it. However, it also exposes some ways to override the behavior. Specifically, adding FORCE_COLOR=true will force chalk to color the text.
So I'm going to see if we can add this capability to concurrency.
Until that time, if you simply add a process.env.FORCE_COLOR = true to your package-scripts.js it will force chalk colors to show.
However, we were using colors to color the output from nps (like nps executing: script string). So this PR replaces colors with the more popular (and widely supported) chalk module. Was pretty much just a find/replace operation.

Fun stuff!

codecov-io · 2017-02-07T18:24:09Z

Codecov Report

Merging #94 into next will not change coverage.

@@         Coverage Diff         @@
##           next    #94   +/-   ##
===================================
  Coverage   100%   100%           
===================================
  Files        11     11           
  Lines       260    237   -23     
===================================
- Hits        260    237   -23

Impacted Files	Coverage Δ
src/index.js	`100% <100%> (ø)`	✅
src/bin-utils/index.js	`100% <100%> (ø)`	✅

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 3566a15...79b8518. Read the comment docs.

kentcdodds · 2017-02-07T18:36:51Z

I should probably also mention that all the trouble I went through in the previous comment was so we could have even more functionality with concurrently than we currently have now with --parallel. We don't have to do anything special to get parody functionality with concurrently.

kentcdodds · 2017-02-08T16:13:40Z

Ok, based on the voting, I think we're going to make this happen. Regardless of whether you're in favor, I appreciate any input on how we can make this change as successful as possible. What can I change about this PR to improve things? Here are a few ideas:

Add the concurrent function (from EXAMPLES.md to the README.md) to inspire people to use JavaScript to make their scripts file cleaner and demonstrate that using concurrently doesn't have to be verbose.
Add a few helper functions people can require into their package-scripts.js to make doing this easier? Maybe? I'm not 100% sure I'm in favor of this.

Any other ideas?

Also, because this is a breaking change, I've set the base branch to next which is where I'll put any breaking changes we want to have go out at the same time. If anyone can think of any other improvements that would result in a breaking change, let me know! I want to avoid doing these major version bumps where possible. The plan is to merge things into next and when we're good and ready, we'll make a PR from next to master, merge it, and it'll get automatically released ✨

kentcdodds · 2017-02-08T16:29:58Z

Just had an idea. If we see the --parallel flag, we should log a warning indicating that this was changed and linking to ERRORS_AND_WARNINGS.md which will describe how to get parallel capabilities. Then we can remove this warning in the next major version bump.

**What**: This removes the `--parallel` capabities from `p-s` in favor of encouraging the use of the `concurrently` module. **Why**: DOTADIW. We want `p-s` to stay as small and focused as possible **How**: Refactoring and documenting BREAKING CHANGE: you must now use `concurrently` to have scripts run in parallel. Check the docs for an example of how to do this.

kentcdodds · 2017-02-09T18:35:46Z

With all the changes I'm making for #70, I'm going to do the --parallel warning later. I'll merge this into next now and keep working on #70 and #100.

**What**: This removes the `--parallel` capabities from `p-s` in favor of encouraging the use of the `concurrently` module. **Why**: DOTADIW. We want `p-s` to stay as small and focused as possible **How**: Refactoring and documenting BREAKING CHANGE: you must now use `concurrently` to have scripts run in parallel. Check the docs for an example of how to do this.

kentcdodds mentioned this pull request Feb 6, 2017

Colors from child scripts not being used in logged output open-cli-tools/concurrently#86

Closed

kentcdodds mentioned this pull request Feb 7, 2017

chore(deps): upgrade deps and use ^ #93

Merged

sxn approved these changes Feb 7, 2017

View reviewed changes

tleunen reviewed Feb 7, 2017

View reviewed changes

kentcdodds force-pushed the pr/remove-parallel branch from 0e79875 to 7ff1c6b Compare February 7, 2017 18:09

kentcdodds force-pushed the pr/remove-parallel branch from 7ff1c6b to eafa9a5 Compare February 7, 2017 18:19

kentcdodds added the don't merge yet label Feb 7, 2017

kentcdodds changed the base branch from master to next February 8, 2017 16:06

kentcdodds changed the base branch from next to master February 8, 2017 16:08

kentcdodds changed the base branch from master to next February 8, 2017 16:09

kentcdodds force-pushed the pr/remove-parallel branch from eafa9a5 to 79b8518 Compare February 9, 2017 17:02

kentcdodds force-pushed the pr/remove-parallel branch from 79b8518 to b315f48 Compare February 9, 2017 18:31

kentcdodds merged commit 296bfac into next Feb 9, 2017

kentcdodds deleted the pr/remove-parallel branch February 9, 2017 18:37

kentcdodds removed the don't merge yet label Mar 9, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

remove parallel #94

remove parallel #94

kentcdodds commented Feb 6, 2017

tleunen commented Feb 6, 2017

kentcdodds commented Feb 6, 2017

kentcdodds commented Feb 7, 2017 •

edited

Loading

sxn Feb 7, 2017

sxn Feb 7, 2017

kentcdodds Feb 7, 2017

sxn commented Feb 7, 2017

tleunen Feb 7, 2017

beeman Feb 7, 2017

kentcdodds Feb 7, 2017

kentcdodds Feb 7, 2017

gunnx commented Feb 7, 2017 via email

kentcdodds commented Feb 7, 2017

kentcdodds commented Feb 7, 2017

kentcdodds commented Feb 7, 2017

codecov-io commented Feb 7, 2017 •

edited

Loading

kentcdodds commented Feb 7, 2017

kentcdodds commented Feb 8, 2017

kentcdodds commented Feb 8, 2017 •

edited

Loading

kentcdodds commented Feb 9, 2017

remove parallel #94

remove parallel #94

Conversation

kentcdodds commented Feb 6, 2017

tleunen commented Feb 6, 2017

kentcdodds commented Feb 6, 2017

kentcdodds commented Feb 7, 2017 • edited Loading

sxn Feb 7, 2017

Choose a reason for hiding this comment

sxn Feb 7, 2017

Choose a reason for hiding this comment

kentcdodds Feb 7, 2017

Choose a reason for hiding this comment

sxn commented Feb 7, 2017

tleunen Feb 7, 2017

Choose a reason for hiding this comment

beeman Feb 7, 2017

Choose a reason for hiding this comment

kentcdodds Feb 7, 2017

Choose a reason for hiding this comment

kentcdodds Feb 7, 2017

Choose a reason for hiding this comment

gunnx commented Feb 7, 2017 via email

kentcdodds commented Feb 7, 2017

kentcdodds commented Feb 7, 2017

kentcdodds commented Feb 7, 2017

codecov-io commented Feb 7, 2017 • edited Loading

Codecov Report

kentcdodds commented Feb 7, 2017

kentcdodds commented Feb 8, 2017

kentcdodds commented Feb 8, 2017 • edited Loading

kentcdodds commented Feb 9, 2017

kentcdodds commented Feb 7, 2017 •

edited

Loading

codecov-io commented Feb 7, 2017 •

edited

Loading

kentcdodds commented Feb 8, 2017 •

edited

Loading