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

feat(server/client): made progress option available to API #1961

Merged
merged 24 commits into from
Jul 1, 2019
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
24 commits
Select commit Hold shift + click to select a range
4a51af2
feat(server/client): made progress option available to API
knagaitsev Jun 4, 2019
f956b3f
test(client): switched snapshot test to single line confirmation
knagaitsev Jun 4, 2019
0ccfce1
refactor(server): removed this.profile
knagaitsev Jun 4, 2019
ef85039
test(client): fixed progress test css path
knagaitsev Jun 4, 2019
94ef8e7
test(client): remove jest timeout
knagaitsev Jun 4, 2019
96c9cd8
test(e2e): change how use of progress on client checked
knagaitsev Jun 11, 2019
adbb746
test(e2e): moved css unlink into afterAll
knagaitsev Jun 11, 2019
10a98d3
test(e2e): use port assigner
knagaitsev Jun 11, 2019
3744482
test(client): add full progress snapshot
knagaitsev Jun 13, 2019
b66dd47
test(client): reg exp progress test
knagaitsev Jun 14, 2019
e688449
test(client): check end of progress updates in console
knagaitsev Jun 14, 2019
fe1db5d
test(client): more generalized test reg exp
knagaitsev Jun 14, 2019
08f79ff
test(client): test to isolate ci problem
knagaitsev Jun 14, 2019
33bf227
test(client): made custom progress plugin to test
knagaitsev Jun 15, 2019
357df68
test(client): new progress plugin multi handler test
knagaitsev Jun 15, 2019
ce5603e
test(client): more testing to identify progress problem
knagaitsev Jun 15, 2019
ea99c9f
test(client): test with 1 progress plugin
knagaitsev Jun 15, 2019
9d9aa24
test(client): new console.log to test sending of data
knagaitsev Jun 15, 2019
ad7a5b5
feat(server): re-add two progress plugins
knagaitsev Jun 15, 2019
8faed38
test(client): revert to original changes
knagaitsev Jun 16, 2019
4741e21
test(progress): added progress and profile option tests
knagaitsev Jun 24, 2019
0b898ec
fix(test): fix profile test port map
knagaitsev Jun 24, 2019
0eaae0f
Merge branch 'master' into progress-option
hiroppy Jul 1, 2019
ae7d586
Merge branch 'master' into progress-option
hiroppy Jul 1, 2019
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
4 changes: 4 additions & 0 deletions bin/options.js
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,10 @@ const options = {
describe:
'Inline mode (set to false to disable including client scripts like livereload)',
},
profile: {
type: 'boolean',
describe: 'Print compilation profile data for progress steps',
},
progress: {
type: 'boolean',
describe: 'Print compilation progress in percentage',
Expand Down
6 changes: 0 additions & 6 deletions bin/webpack-dev-server.js
Original file line number Diff line number Diff line change
Expand Up @@ -97,12 +97,6 @@ function startDevServer(config, options) {
throw err;
}

if (options.progress) {
new webpack.ProgressPlugin({
profile: argv.profile,
}).apply(compiler);
}

try {
server = new Server(compiler, options, log);
} catch (err) {
Expand Down
26 changes: 14 additions & 12 deletions lib/Server.js
Original file line number Diff line number Diff line change
Expand Up @@ -154,23 +154,25 @@ class Server {
}

setupProgressPlugin() {
const progressPlugin = new webpack.ProgressPlugin(
(percent, msg, addInfo) => {
percent = Math.floor(percent * 100);
// for CLI output
new webpack.ProgressPlugin({
profile: !!this.options.profile,
}).apply(this.compiler);
Copy link
Member

Choose a reason for hiding this comment

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

hm, we really need two plugins? I think we can use one instance of plugin

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I think 2 are needed since one supplies a custom handler but the other uses the default handler: https://github.com/webpack/webpack/blob/master/lib/ProgressPlugin.js#L103

Copy link
Member

Choose a reason for hiding this comment

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

hm, we need investigate this and tests this, looks like we setup two progress plugin and it can be invalid usage

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@evilebottnawi Wouldn't it be the equivalent of two plugins tapping into the exact same events? Assuming the plugin was made properly.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@evilebottnawi Are my changes here causing the Windows CI problems? I'm unsure why progress plugin outputs stuff to stderr, but maybe that is causing a problem? https://github.com/webpack/webpack/blob/master/lib/ProgressPlugin.js#L49

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 !! we already validate this only as boolean, so no need convert to boolean again


if (percent === 100) {
msg = 'Compilation completed';
}
// for browser console output
new webpack.ProgressPlugin((percent, msg, addInfo) => {
percent = Math.floor(percent * 100);

if (addInfo) {
msg = `${msg} (${addInfo})`;
}
if (percent === 100) {
msg = 'Compilation completed';
}

this.sockWrite(this.sockets, 'progress-update', { percent, msg });
if (addInfo) {
msg = `${msg} (${addInfo})`;
}
);

progressPlugin.apply(this.compiler);
this.sockWrite(this.sockets, 'progress-update', { percent, msg });
}).apply(this.compiler);
}

setupApp() {
Expand Down
4 changes: 4 additions & 0 deletions lib/options.json
Original file line number Diff line number Diff line change
Expand Up @@ -258,6 +258,9 @@
}
]
},
"profile": {
"type": "boolean"
},
"progress": {
"type": "boolean"
},
Expand Down Expand Up @@ -426,6 +429,7 @@
"pfx": "should be {String|Buffer} (https://webpack.js.org/configuration/dev-server/#devserverpfx)",
"pfxPassphrase": "should be {String} (https://webpack.js.org/configuration/dev-server/#devserverpfxpassphrase)",
"port": "should be {Number|String|Null} (https://webpack.js.org/configuration/dev-server/#devserverport)",
"profile": "should be {Boolean} (https://webpack.js.org/configuration/dev-server/#devserverprofile)",
"progress": "should be {Boolean} (https://webpack.js.org/configuration/dev-server/#devserverprogress---cli-only)",
"proxy": "should be {Object|Array} (https://webpack.js.org/configuration/dev-server/#devserverproxy)",
"public": "should be {String} (https://webpack.js.org/configuration/dev-server/#devserverpublic)",
Expand Down
4 changes: 4 additions & 0 deletions lib/utils/createConfig.js
Original file line number Diff line number Diff line change
Expand Up @@ -47,6 +47,10 @@ function createConfig(config, argv, { port }) {
options.liveReload = false;
}

if (argv.profile) {
options.profile = argv.profile;
}

if (argv.progress) {
options.progress = argv.progress;
}
Expand Down
17 changes: 17 additions & 0 deletions test/cli/cli.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,23 @@ describe('CLI', () => {
.then((output) => {
expect(output.code).toEqual(0);
expect(output.stderr.includes('0% compiling')).toBe(true);
// should not profile
expect(
output.stderr.includes('ms after chunk modules optimization')
).toBe(false);
done();
})
.catch(done);
});

it('--progress --profile', (done) => {
testBin('--progress --profile')
.then((output) => {
expect(output.code).toEqual(0);
// should profile
expect(
output.stderr.includes('ms after chunk modules optimization')
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 better use here snapshot for easy updating, changing string is more difficult

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I will try, last time output did not always match though

Copy link
Member

Choose a reason for hiding this comment

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

We can implement helper for tests what remove unnecessary stuff from output to keep snapshot small and testable

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

We can implement helper for tests what remove unnecessary stuff from output to keep snapshot small and testable

I will try that.

No need !! we already validate this only as boolean, so no need convert to boolean again

It could be undefined though.

Copy link
Member

Choose a reason for hiding this comment

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

Thanks, you are right 👍

).toBe(true);
done();
})
.catch(done);
Expand Down
71 changes: 71 additions & 0 deletions test/e2e/Progress.test.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,71 @@
'use strict';

/* eslint-disable
no-undef
*/
const fs = require('fs');
const { resolve } = require('path');
const testServer = require('../helpers/test-server');
const reloadConfig = require('../fixtures/reload-config/webpack.config');
const runBrowser = require('../helpers/run-browser');
const port = require('../ports-map').Progress;

const cssFilePath = resolve(__dirname, '../fixtures/reload-config/main.css');

describe('client progress', () => {
describe('using hot', () => {
beforeAll((done) => {
fs.writeFileSync(
cssFilePath,
'body { background-color: rgb(0, 0, 255); }'
);
const options = {
port,
host: '0.0.0.0',
inline: true,
hot: true,
progress: true,
watchOptions: {
poll: 500,
},
};
testServer.startAwaitingCompilation(reloadConfig, options, done);
});

afterAll((done) => {
fs.unlinkSync(cssFilePath);
testServer.close(done);
Copy link
Member

Choose a reason for hiding this comment

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

fs.unlinkSync(cssFilePath);

Copy link
Member

Choose a reason for hiding this comment

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

need to add fs.unlinkSync(cssFilePath);

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

done

});

describe('on browser client', () => {
it('should console.log progress', (done) => {
runBrowser().then(({ page, browser }) => {
const res = [];
page.waitForNavigation({ waitUntil: 'load' }).then(() => {
fs.writeFileSync(
cssFilePath,
'body { background-color: rgb(255, 0, 0); }'
);
page.waitFor(10000).then(() => {
browser.close().then(() => {
// check that there is some percentage progress output
const regExp = /^\[WDS\] [0-9]{1,3}% - /;
const match = res.find((line) => {
return regExp.test(line);
});
// eslint-disable-next-line no-undefined
expect(match).not.toEqual(undefined);
done();
});
});
});

page.goto(`http://localhost:${port}/main`);
page.on('console', (data) => {
res.push(data.text());
});
});
});
});
});
});
4 changes: 4 additions & 0 deletions test/options.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -309,6 +309,10 @@ describe('options', () => {
success: ['', 0, null],
failure: [false],
},
profile: {
success: [false],
failure: [''],
},
progress: {
success: [false],
failure: [''],
Expand Down
3 changes: 3 additions & 0 deletions test/ports-map.js
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,9 @@ const portsList = {
WebsocketServer: 1,
ClientMode: 1,
'clientMode-option': 1,
Progress: 1,
'progress-option': 1,
'profile-option': 1,
};

let startPort = 8079;
Expand Down
53 changes: 53 additions & 0 deletions test/server/profile-option.test.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,53 @@
'use strict';

const webpack = require('webpack');
const Server = require('../../lib/Server');
const config = require('../fixtures/simple-config/webpack.config');
const port = require('../ports-map')['profile-option'];

describe('profile', () => {
describe('output', () => {
let mockStderr;

beforeAll(() => {
mockStderr = jest
.spyOn(process.stderr, 'write')
.mockImplementation(() => {});
});

it('should show percentage progress with profile data', (done) => {
const compiler = webpack(config);
const server = new Server(compiler, {
port,
// profile will only have an effect when progress is enabled
progress: true,
profile: true,
});

compiler.hooks.done.tap('webpack-dev-server', () => {
const calls = mockStderr.mock.calls;
mockStderr.mockRestore();
let foundProgress = false;
let foundProfile = false;
calls.forEach((call) => {
if (call[0].includes('0% compiling')) {
foundProgress = true;
}

// this is an indicator that the profile option is enabled,
// so we should expect to find it in stderr since profile is enabled
if (call[0].includes('ms after chunk modules optimization')) {
foundProfile = true;
}
});
expect(foundProgress).toBeTruthy();
expect(foundProfile).toBeTruthy();

server.close(done);
});

compiler.run(() => {});
server.listen(port, 'localhost');
});
});
});
51 changes: 51 additions & 0 deletions test/server/progress-option.test.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,51 @@
'use strict';

const webpack = require('webpack');
const Server = require('../../lib/Server');
const config = require('../fixtures/simple-config/webpack.config');
const port = require('../ports-map')['progress-option'];

describe('progress', () => {
describe('output', () => {
let mockStderr;

beforeAll(() => {
mockStderr = jest
.spyOn(process.stderr, 'write')
.mockImplementation(() => {});
});

it('should show percentage progress without profile data', (done) => {
const compiler = webpack(config);
const server = new Server(compiler, {
port,
progress: true,
});

compiler.hooks.done.tap('webpack-dev-server', () => {
const calls = mockStderr.mock.calls;
mockStderr.mockRestore();
let foundProgress = false;
let foundProfile = false;
calls.forEach((call) => {
if (call[0].includes('0% compiling')) {
foundProgress = true;
}

// this is an indicator that the profile option is enabled,
// so we should expect to not find it in stderr since it is not enabled
if (call[0].includes('ms after chunk modules optimization')) {
foundProfile = true;
}
});
expect(foundProgress).toBeTruthy();
expect(foundProfile).toBeFalsy();

server.close(done);
});

compiler.run(() => {});
server.listen(port, 'localhost');
});
});
});