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: 🐛 do not apply own defaults while setting mode #1565

Merged
merged 13 commits into from
May 23, 2020
Merged
Show file tree
Hide file tree
Changes from 7 commits
Commits
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
11 changes: 8 additions & 3 deletions packages/webpack-cli/__tests__/ZeroConfigGroup.test.js
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
const ZeroConfigGroup = require('../lib/groups/ZeroConfigGroup');

describe('GroupHelper', function () {
it('should load the dev zero config', () => {
describe('ZeroConfigGroup', function () {
it('should load the dev zero config', () => {
const group = new ZeroConfigGroup([
{
dev: true,
Expand All @@ -27,8 +27,9 @@ describe('GroupHelper', function () {
mode: 'production',
},
]);

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

Expand All @@ -40,6 +41,8 @@ describe('GroupHelper', function () {
]);

const result = group.run();
// ensure no other properties are added
expect(result.options).toMatchObject({ mode: 'development' });
Copy link
Member

Choose a reason for hiding this comment

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

Can we look on plugins here?

Copy link
Member Author

@anshumanv anshumanv May 21, 2020

Choose a reason for hiding this comment

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

Old method is only adding config options from the file and no plugins

Copy link
Member

Choose a reason for hiding this comment

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

We should look what we don't have two terser plugins here

Copy link
Member Author

Choose a reason for hiding this comment

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

Can you suggest ways to check it here? We're just passing mode to the compiler, so since core doesn't have two, we won't have two either.

Copy link
Member

@alexander-akait alexander-akait May 21, 2020

Choose a reason for hiding this comment

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

Hm, maybe we can add a test there manually added Terser to minimizer with custom options, for commentsFilename, not the best solution, but will be work

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, something wrong with CI, will take a look tomm

Copy link
Member Author

Choose a reason for hiding this comment

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

@evilebottnawi seems good now, PTAL

expect(result.options.mode).toEqual('development');
});

Expand All @@ -51,6 +54,8 @@ describe('GroupHelper', function () {
]);

const result = group.run();
// ensure no other properties are added
expect(result.options).toMatchObject({ mode: 'none' });
expect(result.options.mode).toEqual('none');
});
});
24 changes: 2 additions & 22 deletions packages/webpack-cli/lib/groups/ZeroConfigGroup.js
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@ class ZeroConfigGroup extends GroupHelper {
* It determines the mode to pass to webpack compiler
* @returns {string} The mode
*/
getEnvFromOptionsAndMode() {
resolveMode() {
if (process.env.NODE_ENV && (process.env.NODE_ENV === PRODUCTION || process.env.NODE_ENV === DEVELOPMENT)) {
return process.env.NODE_ENV;
} else {
Expand Down Expand Up @@ -44,28 +44,8 @@ class ZeroConfigGroup extends GroupHelper {
}
}

resolveZeroConfig() {
const defaultConfigType = this.getEnvFromOptionsAndMode();
let defaultConfig;
if (defaultConfigType === PRODUCTION) {
defaultConfig = require('../utils/production-config')();
} else if (defaultConfigType === DEVELOPMENT) {
defaultConfig = require('../utils/development-config')();
} else {
defaultConfig = require('../utils/none-config')();
}

const isEntryObject = defaultConfig.entry && defaultConfig.entry instanceof Object;
const isOutputDefined = defaultConfig.output && defaultConfig.output.filename;
const isConflictingOutput = isEntryObject && isOutputDefined && defaultConfig.output.filename === 'main.js';
if (isConflictingOutput) {
defaultConfig.output.filename = '[name].main.js';
}
this.opts.options = defaultConfig;
}

run() {
this.resolveZeroConfig();
this.opts.options.mode = this.resolveMode();
return this.opts;
}
}
Expand Down
13 changes: 0 additions & 13 deletions packages/webpack-cli/lib/utils/development-config.js

This file was deleted.

13 changes: 0 additions & 13 deletions packages/webpack-cli/lib/utils/none-config.js

This file was deleted.

13 changes: 0 additions & 13 deletions packages/webpack-cli/lib/utils/production-config.js

This file was deleted.

3 changes: 3 additions & 0 deletions test/mode/mode-with-config/.gitignore
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
yarn.lock
package-lock.json
node_modules/
10 changes: 10 additions & 0 deletions test/mode/mode-with-config/index.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
require("react")
console.log("Ichigo")
if (process.env.NODE_ENV === "production") {
console.log("production mode")
} else if (process.env.NODE_ENV === "development") {
console.log(console.log("development mode"))
} else {
console.log(console.log("none mode"))
}

95 changes: 95 additions & 0 deletions test/mode/mode-with-config/mode-with-config.test.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,95 @@
'use strict';
const { stat, readFile } = require('fs');
const { resolve } = require('path');
// eslint-disable-next-line node/no-unpublished-require
const { run } = require('../../utils/test-utils');

describe.skip('mode flags with config', () => {
it('should run in production mode when --mode=production is passed', (done) => {
anshumanv marked this conversation as resolved.
Show resolved Hide resolved
const { stderr, stdout } = run(__dirname, ['--mode', 'production', '--config', './webpack.config.js']);
expect(stderr).toBeFalsy();
expect(stdout).toBeTruthy();

// Should generate the appropriate files
stat(resolve(__dirname, './bin/main.js.OTHER.LICENSE.txt'), (err, stats) => {
expect(err).toBe(null);
expect(stats.isFile()).toBe(true);
});

stat(resolve(__dirname, './bin/main.js'), (err, stats) => {
expect(err).toBe(null);
expect(stats.isFile()).toBe(true);
});

// Should not generate source maps when not specified
stat(resolve(__dirname, './bin/main.js.map'), (err) => {
expect(err).toBeTruthy();
});

// Correct mode should be propagated to the compiler
readFile(resolve(__dirname, './bin/main.js'), 'utf-8', (err, data) => {
expect(err).toBe(null);
expect(data).toContain('"production mode"');
done();
});
});

it('should run in development mode when --mode=development is passed', (done) => {
const { stderr, stdout } = run(__dirname, ['--mode', 'development', '--config', './webpack.config.js']);
expect(stderr).toBeFalsy();
expect(stdout).toBeTruthy();

// Should generate the appropriate files
stat(resolve(__dirname, './bin/main.js.OTHER.LICENSE.txt'), (err, stats) => {
expect(err).toBe(null);
expect(stats.isFile()).toBe(true);
});

stat(resolve(__dirname, './bin/main.js'), (err, stats) => {
expect(err).toBe(null);
expect(stats.isFile()).toBe(true);
});

// Should not generate source maps when not specified
stat(resolve(__dirname, './bin/main.js.map'), (err) => {
expect(err).toBeTruthy();
});

// Correct mode should be propagated to the compiler
readFile(resolve(__dirname, './bin/main.js'), 'utf-8', (err, data) => {
expect(err).toBe(null);
expect(data).toContain('The "eval" devtool has been used');
expect(data).toContain('development mode');
done();
});
});

it('should run in none mode when --mode=none is passed', (done) => {
const { stderr, stdout } = run(__dirname, ['--mode', 'none', '--config', './webpack.config.js']);
expect(stderr).toBeFalsy();
expect(stdout).toBeTruthy();

// Should generate the appropriate files
stat(resolve(__dirname, './bin/main.js.OTHER.LICENSE.txt'), (err, stats) => {
expect(err).toBe(null);
expect(stats.isFile()).toBe(true);
});

stat(resolve(__dirname, './bin/main.js'), (err, stats) => {
expect(err).toBe(null);
expect(stats.isFile()).toBe(true);
});

// Should not generate source maps when not specified
stat(resolve(__dirname, './bin/main.js.map'), (err) => {
expect(err).toBeTruthy();
});

// Correct mode should be propagated to the compiler
readFile(resolve(__dirname, './bin/main.js'), 'utf-8', (err, data) => {
expect(err).toBe(null);
expect(data).toContain('none mode');
done();
});
});
});
6 changes: 6 additions & 0 deletions test/mode/mode-with-config/package.json
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
{
"dependencies": {
"terser-webpack-plugin": "^3.0.1",
"react": "^16.13.0"
}
}
26 changes: 26 additions & 0 deletions test/mode/mode-with-config/webpack.config.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,26 @@
const path = require('path');
const dirname = __dirname;
const TerserPlugin = require('terser-webpack-plugin');

module.exports = () => {
const config = {
entry: './index.js',
output: {
path: path.join(dirname, 'dist'),
filename: '[name].js',
},
optimization: {
minimizer: [
new TerserPlugin({
sourceMap: false,
extractComments: {
filename: (fileData) => {
return `${fileData.filename}.OTHER.LICENSE.txt${fileData.query}`;
},
},
}),
],
},
};
return config;
};