-
-
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(webpack-cli): to void defaultEntry override the webpack config entry #1289
fix(webpack-cli): to void defaultEntry override the webpack config entry #1289
Conversation
if not pass the entry option through command line, webpack-cli would look for defaultEntry The action would override the entry in webpack config files ISSUES CLOSED: webpack#1288
Thank you for your pull request! The most important CI builds succeeded, we’ll review the pull request soon. |
expect(summary['Output Directory']).toContain(outputDir); | ||
|
||
// eslint-disable-next-line quotes | ||
console.log(stderr); |
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.
Please remove this line.
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.
done
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.
I think the solution should be different. I left some feedback on how to tackle the problem. Let me know what you think
test/entry/config-entry/entry-with-config/entry-with-config.test.js
Outdated
Show resolved
Hide resolved
@Mistyyyy Thanks for your update. I labeled the Pull Request so reviewers will review it again. @ematipico Please review the new changes. |
…onfig ISSUES CLOSED: webpack#1288
To get the correct entry, the entry should use defaultEntry, config entry, command entry in order ISSUES CLOSED: webpack#1288
…tEntry-override-config
After diving into the source code of webpack-merge, I found strategies can't work for the The core code of webpack-merge is here For example const source = {
entry: 'foo'
}
const target = {
entry: 'bar'
}
const strategy = {
entry: 'append', // or replace, the default is append
}
const result = merge.strategy(strategy)(source, target); No matter what strategies are applied, So, if Additional, I can't figure out the effect of the strategies applied to this.strategy = {
'output.filename': 'prepend',
'output.path': 'prepend',
}; I have updated my code, please review that. Thanks. |
Hey @Mistyyyy, thank you very much for your extensive explanation! If it is a bug, I think we should file an issue in webpack-merge repository. If it is intended, this is a a big problem. When I was refactoring the code few weeks back, I was facing a similar issue with |
@Mistyyyy I did a quick try and the strategy actually works... This is the test that I run. Let me know if I missed something. Folder contents of the test
const { resolve } = require('path');
module.exports = {
mode: 'development',
entry: {
index: './file.js',
},
output: {
path: resolve('./dist'),
filename: '[name].bundle.js',
},
};
console.log('js');
'use strict';
const { stat, readFileSync } = require('fs');
const { resolve } = require('path');
const { run } = require('../../utils/test-utils');
describe('default entry and config entry all exist', () => {
it('should use config entry if config entry existed', done => {
const { stdout, stderr } = run(__dirname, ['-c', 'webpack.config.js'], false);
expect(stderr).toBeFalsy();
expect(stdout).toBeTruthy();
stat(resolve(__dirname, './dist/index.bundle.js'), (err, stats) => {
expect(err).toBeFalsy();
expect(stats.isFile()).toBe(true);
done();
});
const data = readFileSync(resolve(__dirname, './dist/index.bundle.js'), 'utf-8');
expect(data).toContain('console.log');
});
}); Changes applied
constructor() {
+ this.strategy = {
+ 'entry': 'prepend'
+ }
}
|
@ematipico Yeah, I think you should add some defaultEntry file which cli can reach. like |
The project structure should look like this
|
Issue found. The problem is inside
resolveFlags() {
const { args } = this;
const { outputOptions, options } = this.opts;
Object.keys(args).forEach(arg => {
if (this.WEBPACK_OPTION_FLAGS.includes(arg)) {
outputOptions[arg] = args[arg];
}
if (arg === 'entry') {
if (args[arg]) {
options.entry = this.resolveFilePath(args[arg], 'index.js');
} else {
options.entry = this.resolveFilePath(args.entry, 'index.js');
}
}
});
// TODO: to add once webpack bundle analyzer is installed, which is not at the moment
// if (arg == 'analyze') {
// const { BundleAnalyzerPlugin } = require('webpack-bundle-analyzer');
// this.opts.options.plugins = [new BundleAnalyzerPlugin()];
// }
if (args.sourcemap) {
options.devtool = args.sourcemap || 'eval';
outputOptions.devtool = args.sourcemap;
}
if (outputOptions['dev']) {
outputOptions['prod'] = undefined;
}
} Your solution was correct but here I basically shifted the responsibility to |
Yes, you are right. It's the and the // The configuration is the result of this.configuration after ConfigGroup executing
const configuration = obj
if (arg === 'entry') {
// if the boolean of args[arg] is truthy, the command entry exists
// it should override the config entry
if (args[arg]) {
options.entry = this.resolveFilePath(args[arg], 'index.js');
} else if (!configuration.entry) {
// the config entry doesn't exist, use defaultEntry
options.entry = this.resolveFilePath(args[arg], 'index.js');
}
} |
I don't think you need the configuration object. I tested that code against your tests and they pass (plus all the current ones pass too) |
if (arg === 'entry') {
if (args[arg]) {
options.entry = this.resolveFilePath(args[arg], 'index.js');
} else {
options.entry = this.resolveFilePath(args.entry, 'index.js');
}
} I think the code above has no difference with the code below, because the if (arg === 'entry') {
options.entry = this.resolveFilePath(args[arg], 'index.js');
} The reason for all tests passes maybe you don't clean the output directory which contain previous test output bundle. I run this test failed. it('should use config entry if config entry existed', done => {
const { stdout } = run(__dirname, ['-c', '../webpack.config.js'], false);
const summary = extractSummary(stdout);
const outputDir = 'entry-with-config/binary';
expect(summary['Output Directory']).toContain(outputDir);
expect(stdout).toContain('./a.js');
stat(resolve(__dirname, './binary/index.bundle.js'), (err, stats) => {
expect(err).toBeFalsy();
expect(stats.isFile()).toBe(true);
done();
});
});
// test Structure
├── a.js
├── entry-with-config
│ ├── entry-with-config.test.js
│ └── index.js
└── webpack.config.js Why I need the configuration object? because the If has config entry, I shouldn't get the defaultEntry and get command entry if In addition, I think Strategy is the best method to fix this issue if webpack-merge supports it. |
You might be right!! And now that I think about the whole logic, I believe your solution, for now, it's the best option! |
@Mistyyyy I merged the PR too soon. Would you mind fixing a failing test? We got rid of the "extractSummary" bit. Sorry man, this was a mistake of mine! |
@ematipico Ok, I will fix it. |
if not pass the entry option through command line, webpack-cli would look for defaultEntry
The defaultEntry would override the entry in webpack config files
ISSUES CLOSED: #1288
What kind of change does this PR introduce?
a bugFix
Did you add tests for your changes?
yes
If relevant, did you update the documentation?
no relevant
#1288
Does this PR introduce a breaking change?
Nothing