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: mode behaviour #1824

Merged
merged 3 commits into from Sep 24, 2020
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
66 changes: 53 additions & 13 deletions packages/webpack-cli/__tests__/resolveMode.test.js
Expand Up @@ -2,41 +2,81 @@ const resolveMode = require('../lib/groups/resolveMode');

describe('resolveMode', function () {
it('should handle the mode option [production]', () => {
const result = resolveMode({
mode: 'production',
});
const result = resolveMode(
{
mode: 'production',
},
{},
);
// ensure no other properties are added
expect(result.options).toMatchObject({ mode: 'production' });
expect(result.options.mode).toEqual('production');
});

it('should handle the mode option [development]', () => {
const result = resolveMode({
mode: 'development',
});
const result = resolveMode(
{
mode: 'development',
},
{},
);

// ensure no other properties are added
expect(result.options).toMatchObject({ mode: 'development' });
expect(result.options.mode).toEqual('development');
});

it('should handle the mode option [none]', () => {
const result = resolveMode({
mode: 'none',
});
const result = resolveMode(
{
mode: 'none',
},
{},
);

// ensure no other properties are added
expect(result.options).toMatchObject({ mode: 'none' });
expect(result.options.mode).toEqual('none');
});

it('should prefer supplied move over NODE_ENV', () => {
it('should prefer supplied move flag over NODE_ENV', () => {
process.env.NODE_ENV = 'production';
const result = resolveMode({
mode: 'development',
});
const result = resolveMode(
{
mode: 'development',
},
{},
);

// ensure no other properties are added
expect(result.options).toMatchObject({ mode: 'development' });
});

it('should prefer supplied move flag over mode from config', () => {
const result = resolveMode(
{
mode: 'development',
},
{ mode: 'production' },
);

// ensure no other properties are added
expect(result.options).toMatchObject({ mode: 'development' });
});

it('should prefer mode form config over NODE_ENV', () => {
process.env.NODE_ENV = 'development';
const result = resolveMode({}, { mode: 'production' });

// ensure no other properties are added
expect(result.options).toMatchObject({ mode: 'production' });
});

it('should prefer mode form flag over NODE_ENV and config', () => {
process.env.NODE_ENV = 'development';
const result = resolveMode({ mode: 'none' }, { mode: 'production' });

// ensure no other properties are added
expect(result.options).toMatchObject({ mode: 'none' });
});
});
47 changes: 35 additions & 12 deletions packages/webpack-cli/lib/groups/resolveMode.js
@@ -1,26 +1,49 @@
const PRODUCTION = 'production';
const DEVELOPMENT = 'development';

const resolveMode = (args) => {
/*
Mode priority:
- Mode flag
- Mode from config
- Mode form NODE_ENV
*/

/**
*
* @param {string} mode - mode flag value
* @param {Object} configObject - contains relevant loaded config
*/
const assignMode = (mode, configObject) => {
const {
env: { NODE_ENV },
} = process;
const { mode } = args;

const { mode: configMode } = configObject;
let finalMode;
/**
* It determines the mode to pass to webpack compiler
* @returns {string} The mode
*/
if (!mode && NODE_ENV && (NODE_ENV === PRODUCTION || NODE_ENV === DEVELOPMENT)) {
if (mode) {
finalMode = mode;
} else if (configMode) {
finalMode = configMode;
} else if (NODE_ENV && (NODE_ENV === PRODUCTION || NODE_ENV === DEVELOPMENT)) {
finalMode = NODE_ENV;
} else {
finalMode = mode || PRODUCTION;
finalMode = PRODUCTION;
Copy link
Member

Choose a reason for hiding this comment

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

No need this, webpack do it itself (default is production)

Copy link
Member Author

Choose a reason for hiding this comment

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

gotcha

Copy link
Member Author

Choose a reason for hiding this comment

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

Need this because #1678 hence added

Copy link
Member

Choose a reason for hiding this comment

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

Okay, let's refactor it in future

Copy link
Member Author

Choose a reason for hiding this comment

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

Sure sure

}
return { mode: finalMode };
};

/**
*
* @param {Object} args - parsedArgs from the CLI
* @param {Object | Array} configOptions - Contains loaded config or array of configs
*/
const resolveMode = (args, configOptions) => {
const { mode } = args;
let resolvedMode;
if (Array.isArray(configOptions)) {
resolvedMode = configOptions.map((configObject) => assignMode(mode, configObject));
} else resolvedMode = assignMode(mode, configOptions);

return {
options: { mode: finalMode },
};
return { options: resolvedMode };
};

module.exports = resolveMode;
21 changes: 18 additions & 3 deletions packages/webpack-cli/lib/webpack-cli.js
Expand Up @@ -77,8 +77,8 @@ class WebpackCLI extends GroupHelper {
this._mergeOptionsToOutputConfiguration(resolvedConfig.outputOptions);
}

async _baseResolver(cb, parsedArgs) {
const resolvedConfig = cb(parsedArgs);
async _baseResolver(cb, parsedArgs, configOptions) {
const resolvedConfig = cb(parsedArgs, configOptions);
this._mergeOptionsToConfiguration(resolvedConfig.options);
this._mergeOptionsToOutputConfiguration(resolvedConfig.outputOptions);
}
Expand Down Expand Up @@ -143,6 +143,21 @@ class WebpackCLI extends GroupHelper {
* @returns {void}
*/
_mergeOptionsToConfiguration(options, strategy) {
/**
* options where they differ per config use this method to apply relevant option to relevant config
* eg mode flag applies per config
*/
if (Array.isArray(options) && Array.isArray(this.compilerConfiguration)) {
this.compilerConfiguration = options.map((option, index) => {
const compilerConfig = this.compilerConfiguration[index];
if (strategy) {
return webpackMerge.strategy(strategy)(compilerConfig, option);
}
return webpackMerge(compilerConfig, option);
});
return;
}

/**
* options is an array (multiple configuration) so we create a new
* configuration where each element is individually merged
Expand Down Expand Up @@ -229,9 +244,9 @@ class WebpackCLI extends GroupHelper {
*/
async runOptionGroups(parsedArgs) {
await Promise.resolve()
.then(() => this._baseResolver(resolveMode, parsedArgs))
.then(() => this._handleDefaultEntry())
.then(() => this._handleConfig(parsedArgs))
.then(() => this._baseResolver(resolveMode, parsedArgs, this.compilerConfiguration))
.then(() => this._handleGroupHelper(this.outputGroup))
.then(() => this._handleCoreFlags())
.then(() => this._handleGroupHelper(this.basicGroup))
Expand Down
6 changes: 6 additions & 0 deletions test/mode/mode-single-arg/mode-single-arg.test.js
Expand Up @@ -26,6 +26,12 @@ describe('mode flags', () => {
expect(stdout).toContain(`mode: 'none'`);
});

it('should pick mode form NODE_ENV', () => {
const { stderr, stdout } = run(__dirname, [], false, [], { NODE_ENV: 'development' });
expect(stderr).toBeFalsy();
expect(stdout).toContain(`mode: 'development'`);
});

it('should throw error when --mode=abcd is passed', () => {
const { stderr } = run(__dirname, ['--mode', 'abcd']);
expect(stderr).toContain('configuration.mode should be one of these');
Expand Down
45 changes: 44 additions & 1 deletion test/mode/mode-with-config/mode-with-config.test.js
Expand Up @@ -92,10 +92,53 @@ describe('mode flags with config', () => {
});
});

it('should use mode from config over flags', () => {
it('should use mode flag over config', () => {
const { stdout, stderr, exitCode } = run(__dirname, ['--mode', 'production', '-c', 'webpack.config2.js']);
expect(stderr).toBeFalsy();
expect(exitCode).toEqual(0);
expect(stdout).toContain(`mode: 'production'`);
});

it('should use mode from flag over NODE_ENV', () => {
const { stdout, stderr, exitCode } = run(__dirname, ['--mode', 'none', '-c', 'webpack.config2.js'], false, [], {
NODE_ENV: 'production',
});
expect(stderr).toBeFalsy();
expect(exitCode).toEqual(0);
expect(stdout).toContain(`mode: 'none'`);
});

it('should use mode from config over NODE_ENV', () => {
const { stdout, stderr, exitCode } = run(__dirname, ['-c', 'webpack.config2.js']);
expect(stderr).toBeFalsy();
expect(exitCode).toEqual(0);
expect(stdout).toContain(`mode: 'development'`);
});

it('should use mode from config when multiple config are supplied', () => {
const { stdout, stderr } = run(__dirname, ['-c', 'webpack.config3.js', '-c', 'webpack.config2.js']);
expect(stderr).toBeFalsy();
expect(stdout).toContain(`mode: 'development'`);
expect(stdout.match(new RegExp("mode: 'development'", 'g')).length).toEqual(1);
});

it('mode flag should apply to all configs', () => {
const { stdout, stderr, exitCode } = run(__dirname, ['--mode', 'none', '-c', './webpack.config3.js', '-c', './webpack.config2.js']);
expect(stderr).toBeFalsy();
expect(exitCode).toEqual(0);
expect(stdout).toContain(`mode: 'none'`);
expect(stdout.match(new RegExp("mode: 'none'", 'g')).length).toEqual(2);
});

it('only config where mode is absent pick up from NODE_ENV', () => {
const { stdout, stderr, exitCode } = run(__dirname, ['-c', './webpack.config3.js', '-c', './webpack.config2.js'], false, [], {
NODE_ENV: 'production',
});
expect(stderr).toBeFalsy();
expect(exitCode).toEqual(0);
expect(stdout).toContain(`mode: 'production'`);
expect(stdout).toContain(`mode: 'development'`);
expect(stdout.match(new RegExp("mode: 'production'", 'g')).length).toEqual(1);
expect(stdout.match(new RegExp("mode: 'development'", 'g')).length).toEqual(1);
});
});
6 changes: 6 additions & 0 deletions test/mode/mode-with-config/webpack.config3.js
@@ -0,0 +1,6 @@
// eslint-disable-next-line node/no-unpublished-require
const WebpackCLITestPlugin = require('../../utils/webpack-cli-test-plugin');

module.exports = {
plugins: [new WebpackCLITestPlugin()],
};
3 changes: 2 additions & 1 deletion test/utils/test-utils.js
Expand Up @@ -19,7 +19,7 @@ const isWebpack5 = version.startsWith('5');
* @param {Boolean} setOutput Boolean that decides if a default output path will be set or not
* @returns {Object} The webpack output or Promise when nodeOptions are present
*/
function run(testCase, args = [], setOutput = true, nodeArgs = []) {
function run(testCase, args = [], setOutput = true, nodeArgs = [], env) {
const cwd = path.resolve(testCase);

const outputPath = path.resolve(testCase, 'bin');
Expand All @@ -29,6 +29,7 @@ function run(testCase, args = [], setOutput = true, nodeArgs = []) {
cwd,
reject: false,
nodeOptions: nodeArgs,
env,
stdio: ENABLE_LOG_COMPILATION ? 'inherit' : 'pipe',
});

Expand Down