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

fix: warn about merge config resolution cases #1674

Merged
merged 10 commits into from
Jul 21, 2020
29 changes: 14 additions & 15 deletions packages/webpack-cli/lib/groups/ConfigGroup.js
Original file line number Diff line number Diff line change
@@ -1,8 +1,10 @@
const { existsSync } = require('fs');
const { resolve, sep, dirname, parse } = require('path');
const webpackMerge = require('webpack-merge');
const { extensions, jsVariants } = require('interpret');
const GroupHelper = require('../utils/GroupHelper');
const rechoir = require('rechoir');
const MergeError = require('../utils/errors/MergeError');

// Order defines the priority, in increasing order
// example - config file lookup will be in order of .webpack/webpack.config.development.js -> webpack.config.development.js -> webpack.config.js
Expand Down Expand Up @@ -84,6 +86,7 @@ class ConfigGroup extends GroupHelper {
const newOptionsObject = {
outputOptions: {},
options: {},
processingMessageBuffer: [],
};

if (!moduleObj) {
Expand Down Expand Up @@ -161,22 +164,18 @@ class ConfigGroup extends GroupHelper {
if (Object.keys(this.args).some((arg) => arg === 'merge')) {
const { merge } = this.args;

const newConfigPath = this.resolveFilePath(merge, 'webpack.base.js');
if (newConfigPath) {
const configFiles = getConfigInfoFromFileName(newConfigPath);
if (!configFiles.length) {
this.opts.processingMessageBuffer.push({
lvl: 'warn',
msg: 'Could not find file to merge configuration with...',
});
return;
}
const foundConfig = configFiles[0];
const resolvedConfig = this.requireConfig(foundConfig);
const newConfigurationsObject = await this.finalize(resolvedConfig);
const webpackMerge = require('webpack-merge');
this.opts['options'] = webpackMerge(this.opts['options'], newConfigurationsObject.options);
// try resolving merge config
const newConfigPath = this.resolveFilePath(merge);

if (!newConfigPath) {
throw new MergeError("The supplied merge config doesn't exist.");
}

const configFiles = getConfigInfoFromFileName(newConfigPath);
const foundConfig = configFiles[0];
const resolvedConfig = this.requireConfig(foundConfig);
const newConfigurationsObject = await this.finalize(resolvedConfig);
this.opts['options'] = webpackMerge(this.opts['options'], newConfigurationsObject.options);
}
}

Expand Down
3 changes: 2 additions & 1 deletion packages/webpack-cli/lib/utils/GroupHelper.js
Original file line number Diff line number Diff line change
Expand Up @@ -47,7 +47,8 @@ class GroupHelper {
const configPathExists = predefinedConfigPath ? existsSync(predefinedConfigPath) : undefined;

if (!configPathExists) {
const LOOKUP_PATHS = [`${filename}`, `src/${filename}`, defaultValue, `src/${defaultValue}`];
const LOOKUP_PATHS = [`${filename}`, `src/${filename}`, ...(defaultValue ? [defaultValue, `src/${defaultValue}`] : [])];

if (filename) {
LOOKUP_PATHS.unshift(filename);
}
Expand Down
10 changes: 10 additions & 0 deletions packages/webpack-cli/lib/utils/errors/MergeError.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
class MergeError extends Error {
constructor(message) {
super(message);
this.name = 'MergeError';
// No need to show stack trace for known errors
this.stack = '';
}
}

module.exports = MergeError;
File renamed without changes.
22 changes: 22 additions & 0 deletions test/merge/config-absent/merge-config-absent.test.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,22 @@
'use strict';

const fs = require('fs');
const { join } = require('path');

const { run } = require('../../utils/test-utils');

describe('merge flag configuration', () => {
it('Show warning message when the merge config is absent', () => {
// 2.js doesn't exist, let's try merging with it
const { stdout, stderr } = run(__dirname, ['--config', './1.js', '--merge', './2.js'], false);

// Since the process will exit, nothing on stdout
expect(stdout).toBeFalsy();
// Confirm that the user is notified
expect(stderr).toContain(`MergeError: The supplied merge config doesn't exist.`);
// Default config would be used
expect(fs.existsSync(join(__dirname, './dist/merged.js'))).toBeFalsy();
// Since the process will exit so no compilation will be done
expect(fs.existsSync(join(__dirname, './dist/main.js'))).toBeFalsy();
anshumanv marked this conversation as resolved.
Show resolved Hide resolved
anshumanv marked this conversation as resolved.
Show resolved Hide resolved
});
});
1 change: 1 addition & 0 deletions test/merge/config-absent/some_entry.js
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
console.log('Oikawa');
5 changes: 4 additions & 1 deletion test/merge/config/1.js
Original file line number Diff line number Diff line change
@@ -1,3 +1,6 @@
module.exports = {
entry: './some_entry.js',
entry: './old_entry.js',
output: {
filename: 'badfile.js',
},
};
1 change: 1 addition & 0 deletions test/merge/config/2.js
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
module.exports = {
entry: './some_entry.js',
output: {
filename: 'merged.js',
},
Expand Down
9 changes: 9 additions & 0 deletions test/merge/config/merge-config.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -15,4 +15,13 @@ describe('merge flag configuration', () => {
done();
});
});
it('merges two configurations together with flag alias', (done) => {
const { stdout } = run(__dirname, ['--config', './1.js', '-m', './2.js'], false);
expect(stdout).toContain('merged.js');
stat(resolve(__dirname, './dist/merged.js'), (err, stats) => {
expect(err).toBe(null);
expect(stats.isFile()).toBe(true);
done();
});
});
Copy link
Member

@alexander-akait alexander-akait Jul 21, 2020

Choose a reason for hiding this comment

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

Let's override the other option, for example filename, too

Copy link
Member Author

Choose a reason for hiding this comment

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

added 1fdf2ae 👍

});
3 changes: 0 additions & 3 deletions test/merge/defaults/1.js

This file was deleted.

39 changes: 0 additions & 39 deletions test/merge/defaults/merge-defaults.test.js

This file was deleted.

1 change: 0 additions & 1 deletion test/merge/defaults/some_entry.js

This file was deleted.

1 change: 0 additions & 1 deletion test/merge/defaults/some_other_entry.js

This file was deleted.

5 changes: 0 additions & 5 deletions test/merge/defaults/webpack.base.js

This file was deleted.