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

enable progress from config #1181

Merged
merged 3 commits into from Dec 22, 2017

Conversation

ryanwholey
Copy link
Contributor

@ryanwholey ryanwholey commented Nov 8, 2017

Enables progress option to be read from the passed config (in addition to the command line)

What kind of change does this PR introduce?
Extends current feature by allowing option to be read from webpack config

Did you add or update the examples/?
The example passed progress in its config already, so it wasnt working properly. It now works as expected.

Summary
I was working on another project which uses webpack dev server heavily. I like the progress option because it gives the user some feedback. On a code review, someone recommended to clean up my package.json I should move the --progress options to the config since I pass it to each webpack-dev-server script. Turned out it was CLI only!

Does this PR introduce a breaking change?
No it should not!

Other information
Very excited to contribute! I really love webpack dev server and I hope I get the opportunity to contribute more in the future.

@codecov
Copy link

codecov bot commented Nov 8, 2017

Codecov Report

Merging #1181 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #1181   +/-   ##
=======================================
  Coverage   76.31%   76.31%           
=======================================
  Files           5        5           
  Lines         477      477           
  Branches      154      154           
=======================================
  Hits          364      364           
  Misses        113      113

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 e8964d1...e84ef2c. Read the comment docs.

@@ -368,7 +370,7 @@ function startDevServer(webpackOptions, options) {
throw e;
}

if (argv.progress) {
Copy link
Contributor

Choose a reason for hiding this comment

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

a less impactful option would have been to set options.progress within the existing if block. keep an eye on those kinds of simple changes in the future and give them favor.

Choose a reason for hiding this comment

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

I think I understand what you're saying, since the progress opt is only used here and its not always common to use, it would have been less impactful to set the progress opt inside of the if block so it wouldn't be assigned on every run?

Thank you for the review! Looking forward to the v3 version :)

@shellscape
Copy link
Contributor

@ryanwholey (copied from your related PR) thanks for the PR! we love getting new tests submitted and normally would be quick to add them to the repo. right now we're in a holding pattern on new code and new features while we prep v3 on the beta branch. While we're not looking for outside contributions on that branch at the moment, we will be in the near future. we're going to have to put this PR on hold for a bit, but we'll be circling back to it.

@shellscape shellscape merged commit 7e89442 into webpack:master Dec 22, 2017
@shellscape
Copy link
Contributor

Thanks for your patience @ryanwholey and your contributions. We've merged your two PRs.

@ryanwholey
Copy link
Contributor Author

nice!!!!! thanks team, happy holidays!

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.

None yet

3 participants