-
-
Notifications
You must be signed in to change notification settings - Fork 602
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
--entry should override config.entry #358
--entry should override config.entry #358
Conversation
Thank you for your pull request! The most important CI builds succeeded, we’ll review the pull request soon. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good, thanks! I'm a bit worried about the removal of the Array checking, but tests are running fine. Could revert if its a problem.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could we also provide a test regarding multiple entry overrides? So we are also sure that providing entry Arrays is also overridden as expected.
@fokusferit this might be implemented already, could you check the binTestCases? |
Also I'm currently thinking about this line:
Is this if-else still necessary or valid? We are forcing that entry is |
It's in another subfolder https://github.com/webpack/webpack-cli/tree/master/test/binCases/entry/multi-file
@ev1stensberg I removed the array check because if we get to that point in the code, we know that entries were passed as CLI arguments, so we force @fokusferit I left the conditional check you referenced in the code because that function as a whole appears to be called for every entry provided via CLI options. I figured we should still allow a downstream user to provide multiple paths for the same entry: webpack-cli --entry bar=./some/path.js --entry bar=./another/path.js Thanks for the review! Happy to rework any of this, just let me know. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@bitpshr thanks for the explanation! Yes, I've just quickly asked myself which reason is left to keep that code and thanks for the use case.
I don't have any comment so, when @ev1stensberg is still fine, we can merge it.
Thanks @ematipico, @fokusferit, and @ev1stensberg. Let me know if you'd like me to clarify or clean up anything else before landing this. |
I'm kinda shocked that no one raised an issue with this PR. This effectively means that users can’t add entries dynamically on the command line, which is the pattern every flag but This PR only exchanges one set of disadvantages for another. Poor choice imho, folks. |
@shellscape I'm not sure I follow; most CLI options passed dynamically at runtime override corresponding configuration properties defined in a configuration file. See While I understand the desire to dynamically add to configuration-file-defined I disagree with the assertion that this PR trades one set of disadvantages for another: this PR makes dynamically adding to entries slightly more tedious so that overriding entries is at all possible, and does so by handling |
I'm not sure you have a full handle on the scope of the CLI options available, because "most" is not a true assertion. By your logic you should then make all flags that modify the
That illustrates my point perfectly - that is literally trading one inconvenience for another. This PR was not forward thinking, and merged with tunnel vision for only one way of doing things, whilst ignoring the other. It's a failure of vision and an architectural mistake. |
Ah, so we handle any modifications to the Is
Thanks for bringing up up your concern. I worry we can only be so consistent when translating CLI options to configuration options, with |
What kind of change does this PR introduce?
Feature
Did you add tests for your changes?
Yes
If relevant, did you update the documentation?
Yes
Summary
It should be possible to use a configuration file while also overridding specific parameters using CLI options. This is already possible for most configuration options. For example, the CLI will ignore any
output
configured in a project'swebpack.config.js
and instead use the value of--output
if it's passed. This allows a configuration file to be used while also allowing certain options to be overridden dynamically at runtime. Unfortunately, the current implementation doesn't handleentry
in this manner. Instead, the CLI will append the value of any--entry
option to theentry
configuration in a project'swebpack.config.js
instead of overriding, which prevents projects from dynamically overridingentry
points using CLI options.This PR updates the CLI to handle
--entry
the same way as--output
: any entry points specified via CLI options overrideentry
configuration specified in a configuration file, rather than appending to it.Resolves #155
Example
webpack.config.js
Should create one
foo
entry based on config file:Should override config file and create one
bar
entry:or
Does this PR introduce a breaking change?
Eh, sort of. If projects were previously relying on the undocumented functionality where CLI-passed entry points were appended to configuration-defined entry points, they will break with this release. I'd argue that since the previous functionality wasn't explicit or documented that it doesn't warrant a major version bump.
Other information
This PR also updates the usage help text printed by the CLI since it wasn't exactly accurate. Using CLI options is not mutually-exclusive to using a configuration file, and an
entry
point can be dynamically defined as either a CLI option or a CLI argument.