-
-
Notifications
You must be signed in to change notification settings - Fork 597
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
fix: support top multi compiler options #2874
Conversation
const evaluatedConfigs = await Promise.all( | ||
options.config.map(async (value) => | ||
evaluateConfig(await loadConfig(path.resolve(value)), options.argv || {}), | ||
const loadedConfig = await Promise.all( |
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.
const loadedConfig = await Promise.all( | |
const loadedConfigs = await Promise.all( |
loadedConfig.forEach((loadedConfig) => { | ||
const isArray = Array.isArray(loadedConfig.options); | ||
|
||
// TODO we should run webpack multiple times when the `--config` options has multiple values without `--merge`, need to solve for the next major release |
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.
// TODO we should run webpack multiple times when the `--config` options has multiple values without `--merge`, need to solve for the next major release | |
// TODO we should run webpack multiple times when the `--config` options have multiple values with `--merge`, need to solve for the next major release |
evaluatedConfig.options.forEach((options) => { | ||
config.options.push(options); | ||
config.path.set(options, evaluatedConfig.path); | ||
loadedConfig.forEach((loadedConfig) => { |
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.
loadedConfig.forEach((loadedConfig) => { | |
loadedConfigs.forEach((loadedConfig) => { |
Codecov Report
@@ Coverage Diff @@
## master #2874 +/- ##
==========================================
- Coverage 95.01% 94.95% -0.07%
==========================================
Files 31 31
Lines 1706 1704 -2
Branches 498 484 -14
==========================================
- Hits 1621 1618 -3
- Misses 85 86 +1
Continue to review full report at Codecov.
|
/cc @webpack/cli-team need review |
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.
LGTM
async loadConfig(configPath, argv = {}) { | ||
const { interpret } = this.utils; | ||
const ext = path.extname(configPath); | ||
const interpreted = Object.keys(interpret.jsVariants).find((variant) => variant === ext); |
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.
can just use interpret.jsVariants[ext]
?
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.
oh it is existing logic, maybe we can improve it late
What kind of change does this PR introduce?
bugfix
Did you add tests for your changes?
yes
If relevant, did you update the documentation?
No
Summary
fixes #2873
Does this PR introduce a breaking change?
No
Other information
We should refactor our logic for multiple
--config
, ideally I think we should runmerge
when multiple configurations provided, but it is breaking change and should be solved for the next major release