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

cli: fix watch options for array config #892

Merged
merged 1 commit into from May 27, 2019

Conversation

@chaseadamsio
Copy link
Contributor

chaseadamsio commented May 24, 2019

What kind of change does this PR introduce?
bugfix

Did you add tests for your changes?
nope! I didn't see any existing tests for processOptions.

If relevant, did you update the documentation?
I did not, but this should make the current docs on the watch options true for Webpack configs that are arrays.

Summary

webpack/webpack#4594 points out that watch options aren't being honored when the
Webpack config is an Array. In cases when it's an array, there may not
be a firstOptions.watchOptions, but there will be an options.watchOptions.

In the current implementation, when the Webpack compiler watch method is
called with watchOptions and the Webpack config is an array, watchOptions
will be true and not an object.

I should also note that I don't have a ton of context into the intentions behind using the watch (which is a boolean) in the case that the other options are absent, and an object if watch is false on both options and firstOptions, so at the very least my goal with this PR is to give someone who has better context the information I found so that they can make a good decision on what's best to do here.

webpack/webpack#4594 (not open, but still broken)

  • I also noticed that although the above issue was specific to docker + Windows 10, this isn't working as expected in any system I've tested (in a Docker container, locally on MacOS) when the Webpack config export is an array.

Does this PR introduce a breaking change?

It does not, this should be a seamless fallback in the case when the Webpack config is an array instead of an object that does the right thing when the CLI has watch-* flags.

Testing/Repro

yarn run webpack-watch-poll-cfg
yarn run v1.9.4
$ webpack --watch --config webpack-poll.config.js
watch options in compiler: { poll: 1000 }
  • Ran webpack-watch-poll-cli from the example repo to verify the watchOptions from a CLI flag (--watch-poll) was incorrect (with an Array config):
yarn webpack-watch-poll-cli
yarn run v1.9.4
$ webpack --watch --watch-poll=500 --config webpack-basic.config.js
watch options in compiler: true
watch options in compiler: true
yarn webpack-watch-poll-cli
yarn run v1.9.4
$ webpack --watch --watch-poll=500 --config webpack-basic.config.js
watch options in compiler: { poll: 500 }
@jsf-clabot
Copy link

jsf-clabot commented May 24, 2019

CLA assistant check
All committers have signed the CLA.

@webpack-bot
Copy link

webpack-bot commented May 24, 2019

Thank you for your pull request! The most important CI builds succeeded, we’ll review the pull request soon.

@anshumanv
Copy link
Contributor

anshumanv commented May 24, 2019

Please sign the CLA :)

@chaseadamsio
Copy link
Contributor Author

chaseadamsio commented May 24, 2019

@anshumanv per the CLA assistant page I have signed it (I did so as soon as I opened the PR), here's a screenshot of what I see when I visit the link from here (with my email obfuscated):
image

Any tips on what I can do to reset and re-sign so that it shows correctly are appreciated!

Update: it looks like others have seen this too, I've opened an issue to see if I can figure out what happened and get it squared away here. I force pushed a rebase & that did the trick.

@anshumanv
Copy link
Contributor

anshumanv commented May 24, 2019

Thanks @chaseadamsio, can you try closing and reopening this PR?
I think it will run a fresh check on doing so.

Issue #4594 points out that watch options aren't being honored when the
Webpack config is an Array. In cases when it's an array, there may not
be a firstOptions.watchOptions, but there will be an options.watchOptions.

In the current implementation, when the Webpack compiler watch method is
called with watchOptions and the Webpack config is an array, watchOptions
will be `true` and not an object.
@chaseadamsio chaseadamsio force-pushed the chaseadamsio-forks:fixWatchOptions branch from 91239ef to bb675fc May 24, 2019
@anshumanv
Copy link
Contributor

anshumanv commented May 24, 2019

It worked! 💯

Copy link
Member

evenstensberg left a comment

lgtm, thank you so much for the Pull Request. Well thorough Pull Request description and elaboration.

@ematipico ematipico merged commit 0c97480 into webpack:master May 27, 2019
13 checks passed
13 checks passed
Codacy/PR Quality Review Up to standards. A positive pull request.
Details
commitlint found 0 problems, 0 warnings
continuous-integration/travis-ci/pr The Travis CI build passed
Details
licence/cla Contributor License Agreement is signed.
Details
security/snyk - package.json (evenstensberg) No manifest changes detected
security/snyk - package.json (sokra) No manifest changes detected
webpack.webpack-cli Build #20190524.5 succeeded
Details
webpack.webpack-cli (Linux node-10) Linux node-10 succeeded
Details
webpack.webpack-cli (Linux node-12) Linux node-12 succeeded
Details
webpack.webpack-cli (Linux node-8) Linux node-8 succeeded
Details
webpack.webpack-cli (macOS node-10) macOS node-10 succeeded
Details
webpack.webpack-cli (macOS node-12) macOS node-12 succeeded
Details
webpack.webpack-cli (macOS node-8) macOS node-8 succeeded
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

7 participants
You can’t perform that action at this time.