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.

Sign up for GitHub

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

Jump to bottom

fix: set mode=production by default #1688

Merged
merged 2 commits into from
Jul 17, 2020
Merged

fix: set mode=production by default #1688

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
0ca97e7
fix: set mode=production by default
snitin315 Jul 17, 2020
b4d0434
Merge branch 'next' into fix/default-mode
snitin315 Jul 17, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 8 additions & 0 deletions packages/webpack-cli/__tests__/ZeroConfigGroup.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -86,4 +86,12 @@ describe('ZeroConfigGroup', function () {
// ensure no other properties are added
expect(result.options).toMatchObject({ mode: 'development' });
});

it('should set mode=production by default', () => {
const group = new ZeroConfigGroup([{}]);

const result = group.run();
// ensure no other properties are added
expect(result.options).toMatchObject({ mode: 'production' });
});
});
4 changes: 2 additions & 2 deletions packages/webpack-cli/lib/utils/cli-flags.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -191,7 +191,7 @@ module.exports = {
usage: '--dev',
alias: 'd',
type: Boolean,
defaultValue: undefined,
defaultValue: false,
group: ZERO_CONFIG_GROUP,
description: 'Run development build',
link: 'https://webpack.js.org/concepts/#mode',
Expand All @@ -201,7 +201,7 @@ module.exports = {
alias: 'p',
usage: '--prod',
type: Boolean,
defaultValue: undefined,
defaultValue: false,
Copy link
Member

Choose a reason for hiding this comment

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

I've always wondered why we have flags --dev/--prod, we already have --mode, it can be misleading for new developers and some of them will be write webpack --prod --mode=production, but it is wrong

Copy link
Member Author

@snitin315 snitin315 Jul 17, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

I've always wondered why we have flags --dev/--prod, we already have --mode, it can be misleading for new developers and some of them will be write webpack --prod --mode=production, but it is wrong

It's just alias for --mode=development or production like we have --verbose for --stats=verbose.
CLI throws a warning if user do something like webpack --prod --mode=production
You provided both 'mode' and --prod arguments. You should provide just one. "mode" will be used

Copy link
Member

Choose a reason for hiding this comment

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

I think we should remove them in the future, but it is not high priority

Copy link
Member Author

Choose a reason for hiding this comment

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

/cc @webpack/cli-team anyone in favor of --dev and --prod.?

Not a big deal but even if I understand the point of having clean and concise --dev and --prod options, I think the --mode= option better matches the way it works in configuration file and is more straightforward when you already know configuration file. Just my 2 cents, but I'm not sure a breaking change in this area is really useful ?

from feedback too #1222 (comment)

group: ZERO_CONFIG_GROUP,
description: 'Run production build',
link: 'https://webpack.js.org/concepts/#mode',
Expand Down
4 changes: 2 additions & 2 deletions test/defaults/output-defaults.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,8 +6,8 @@ const { run } = require('../utils/test-utils');
describe('output flag defaults', () => {
it('should create default file for a given directory', (done) => {
const { stdout } = run(__dirname, ['--entry', './a.js', '--output', './binary'], false);
// Should print a warning about config fallback
expect(stdout).toContain('option has not been set, webpack will fallback to');
// Should not print warning about config fallback, as we have production as default
expect(stdout).not.toContain('option has not been set, webpack will fallback to');
stat(resolve(__dirname, './binary/main.js'), (err, stats) => {
expect(err).toBe(null);
expect(stats.isFile()).toBe(true);
Expand Down
2 changes: 1 addition & 1 deletion test/merge/config/merge-config.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@ const { run } = require('../../utils/test-utils');
describe('merge flag configuration', () => {
it('merges two configurations together', (done) => {
const { stdout } = run(__dirname, ['--config', './1.js', '--merge', './2.js'], false);
expect(stdout).toContain('option has not been set, webpack will fallback to');
expect(stdout).not.toContain('option has not been set, webpack will fallback to');
stat(resolve(__dirname, './dist/merged.js'), (err, stats) => {
expect(err).toBe(null);
expect(stats.isFile()).toBe(true);
Expand Down
18 changes: 15 additions & 3 deletions test/merge/defaults/merge-defaults.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,27 +1,39 @@
'use strict';

const { stat } = require('fs');
const { stat, readFile } = require('fs');
const { resolve } = require('path');

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

describe('merge flag defaults', () => {
it('merges a default webpack.base.config with default config lookup', (done) => {
const { stdout } = run(__dirname, ['-m', './'], false);
expect(stdout).toContain('option has not been set, webpack will fallback to');
expect(stdout).not.toContain('option has not been set, webpack will fallback to');

stat(resolve(__dirname, './dist/default.js'), (err, stats) => {
expect(err).toBe(null);
expect(stats.isFile()).toBe(true);
done();
});
readFile(resolve(__dirname, './dist/default.js'), 'utf-8', (err, data) => {
expect(err).toBe(null);
expect(data).toContain('some_entry');
done();
});
});
it('merges a configuration file with default base config', (done) => {
const { stdout } = run(__dirname, ['-c', './1.js'], false);
expect(stdout).toContain('option has not been set, webpack will fallback to');

expect(stdout).not.toContain('option has not been set, webpack will fallback to');
stat(resolve(__dirname, './dist/default.js'), (err, stats) => {
expect(err).toBe(null);
expect(stats.isFile()).toBe(true);
done();
});
readFile(resolve(__dirname, './dist/default.js'), 'utf-8', (err, data) => {
expect(err).toBe(null);
expect(data).toContain('some_entry');
done();
});
});
});
2 changes: 1 addition & 1 deletion test/merge/defaults/some_entry.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1 +1 @@
console.log('<3');
console.log('some_entry');
2 changes: 1 addition & 1 deletion test/merge/defaults/some_other_entry.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1 +1 @@
console.log('moo');
console.log('some_other_entry');
31 changes: 27 additions & 4 deletions test/mode/mode-single-arg/mode-single-arg.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,10 +3,27 @@ const { run } = require('../../utils/test-utils');
const { stat, readFile } = require('fs');
const { resolve } = require('path');
describe('mode flags', () => {
it('should set mode=production by default', (done) => {
const { stderr, stdout } = run(__dirname);
expect(stderr).toBeFalsy();
expect(stdout).toContain(`mode: 'production'`);

stat(resolve(__dirname, './bin/main.js'), (err, stats) => {
expect(err).toBe(null);
expect(stats.isFile()).toBe(true);
done();
});
readFile(resolve(__dirname, './bin/main.js'), 'utf-8', (err, data) => {
expect(err).toBe(null);
expect(data).toContain('production test');
done();
});
});

it('should load a development config when --mode=development is passed', (done) => {
const { stderr, stdout } = run(__dirname, ['--mode', 'development']);
expect(stderr).toBeFalsy();
expect(stdout).toBeTruthy();
expect(stdout).toContain(`mode: 'development'`);

stat(resolve(__dirname, './bin/main.js'), (err, stats) => {
expect(err).toBe(null);
Expand All @@ -18,19 +35,24 @@ describe('mode flags', () => {
it('should load a production config when --mode=production is passed', (done) => {
const { stderr, stdout } = run(__dirname, ['--mode', 'production']);
expect(stderr).toBeFalsy();
expect(stdout).toBeTruthy();
expect(stdout).toContain(`mode: 'production'`);

stat(resolve(__dirname, './bin/main.js'), (err, stats) => {
expect(err).toBe(null);
expect(stats.isFile()).toBe(true);
done();
});
readFile(resolve(__dirname, './bin/main.js'), 'utf-8', (err, data) => {
expect(err).toBe(null);
expect(data).toContain('production test');
done();
});
});

it('should load a none config when --mode=none is passed', (done) => {
const { stderr, stdout } = run(__dirname, ['--mode', 'none']);
expect(stderr).toBeFalsy();
expect(stdout).toBeTruthy();
expect(stdout).toContain(`mode: 'none'`);

stat(resolve(__dirname, './bin/main.js'), (err, stats) => {
expect(err).toBe(null);
Expand All @@ -42,7 +64,8 @@ describe('mode flags', () => {
it('should show warning and load a production config when --mode=abcd is passed', (done) => {
const { stderr, stdout } = run(__dirname, ['--mode', 'abcd']);
expect(stderr).toContain('invalid value for "mode" option. Using "production" by default');
expect(stdout).toBeTruthy();
expect(stdout).toContain(`mode: 'production'`);

stat(resolve(__dirname, './bin/main.js'), (err, stats) => {
expect(err).toBe(null);
expect(stats.isFile()).toBe(true);
Expand Down
7 changes: 7 additions & 0 deletions test/mode/mode-single-arg/webpack.config.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
// eslint-disable-next-line node/no-unpublished-require
const WebpackCLITestPlugin = require('../../utils/webpack-cli-test-plugin');

module.exports = {
entry: './src/index.js',
plugins: [new WebpackCLITestPlugin()],
};