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

Conversation

knagaitsev
Copy link
Collaborator

  • This is a bugfix
  • This is a feature
  • This is a code refactor
  • This is a test update
  • This is a docs update
  • This is a metadata update

For Bugs and Features; did you add new tests?

Yes

Motivation / Use-Case

GSoC Goal: #1960

The main goal of this is to remove functionality from bin/webpack-dev-server and into the API. This is the first step, moving ProgressPlugin usage over to the API.

As a side effect of this, I discovered that the CLI-specific part was using argv.profile. To not make this a breaking change, I added a profile option as well, so that it can get passed into the API.

Breaking Changes

Despite not being documented, you could use the progress option on API before. However, it only outputted progress to the client console. Now, progress is also outputted to the command line.

Additional Info

New documentation will be needed for profile option, along with changes to progress option

@codecov
Copy link

codecov bot commented Jun 4, 2019

Codecov Report

Merging #1961 into master will increase coverage by 0.97%.
The diff coverage is 66.66%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1961      +/-   ##
==========================================
+ Coverage   92.77%   93.75%   +0.97%     
==========================================
  Files          29       29              
  Lines        1149     1152       +3     
  Branches      327      328       +1     
==========================================
+ Hits         1066     1080      +14     
+ Misses         79       70       -9     
+ Partials        4        2       -2
Impacted Files Coverage Δ
lib/Server.js 95.16% <100%> (+2.65%) ⬆️
lib/utils/createConfig.js 91.07% <50%> (-0.75%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f611304...279077f. Read the comment docs.

@knagaitsev
Copy link
Collaborator Author

Need to change paths for new test after this is merged: #1958

});

afterAll((done) => {
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

Copy link
Member

@alexander-akait alexander-akait left a comment

Choose a reason for hiding this comment

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

Need resolve couple problems

lib/Server.js Outdated
@@ -91,6 +91,7 @@ class Server {
// TODO this.<property> is deprecated (remove them in next major release.) in favor this.options.<property>
this.hot = this.options.hot || this.options.hotOnly;
this.headers = this.options.headers;
this.profile = !!this.options.profile;
Copy link
Member

Choose a reason for hiding this comment

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

this.options.profile, don't use this.<option> it is unnecessary and all this.<option> will be removed in next major release

@@ -147,6 +148,10 @@ class Server {
}

setupProgressPlugin() {
new webpack.ProgressPlugin({
profile: this.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

});

describe('on browser client', () => {
jest.setTimeout(30000);
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 delete after #1965 is merged

@knagaitsev
Copy link
Collaborator Author

What is wrong with Windows CI?

@alexander-akait
Copy link
Member

Something leak on windows due jest freeze, we need investigate this

@knagaitsev
Copy link
Collaborator Author

I think the problem is that ProgressPlugin probably does not guarantee the same output each time, so I should do a more generalized test on e2e client to see that it is outputting some progress data. I think the hanging can be solved via the solution here: #2018

'body { background-color: rgb(0, 0, 255); }'
);
const options = {
port: 9000,
Copy link
Member

Choose a reason for hiding this comment

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

this test should use a port variable

});

afterAll((done) => {
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.

need to add fs.unlinkSync(cssFilePath);

return testExp.test(line);
});
// eslint-disable-next-line no-undefined
expect(match).not.toEqual(undefined);
Copy link
Member

Choose a reason for hiding this comment

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

Maybe using snapshots for this?

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 It is not outputting the same thing each time, like differences in numbers and such.

Copy link
Member

@alexander-akait alexander-akait left a comment

Choose a reason for hiding this comment

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

Two notes. Good work

@@ -147,6 +148,10 @@ class Server {
}

setupProgressPlugin() {
new webpack.ProgressPlugin({
profile: this.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.

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

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 👍

@knagaitsev
Copy link
Collaborator Author

Testing with full snapshot again, last time there were some inconsistencies in output for a few CI builds though.

@knagaitsev
Copy link
Collaborator Author

@evilebottnawi take a look at CI, there are minor differences in console output like one line getting printed twice or lines not getting printed. That's why I said snapshot is difficult.

Copy link
Member

@alexander-akait alexander-akait left a comment

Choose a reason for hiding this comment

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

/cc @hiroppy

lib/options.json Outdated
@@ -422,6 +425,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}",
Copy link
Member

Choose a reason for hiding this comment

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

Please put link here to avoid changing in future

@alexander-akait
Copy link
Member

/cc @Loonride hm, okay, let's change this on regexp/string test

@codecov-io
Copy link

codecov-io commented Jun 14, 2019

Codecov Report

Merging #1961 into master will increase coverage by 0.92%.
The diff coverage is 90%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1961      +/-   ##
==========================================
+ Coverage   92.92%   93.84%   +0.92%     
==========================================
  Files          32       32              
  Lines        1201     1202       +1     
  Branches      335      336       +1     
==========================================
+ Hits         1116     1128      +12     
+ Misses         81       72       -9     
+ Partials        4        2       -2
Impacted Files Coverage Δ
lib/Server.js 95.39% <100%> (+2.61%) ⬆️
lib/utils/createConfig.js 91.07% <50%> (-0.75%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f984e53...0b898ec. Read the comment docs.

@knagaitsev
Copy link
Collaborator Author

Hmm still can't pass CI though. I tried to choose a pretty generic RegExp, surprised it is not getting outputted in some cases

@codecov
Copy link

codecov bot commented Jul 1, 2019

Codecov Report

Merging #1961 into master will increase coverage by 0.91%.
The diff coverage is 90%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1961      +/-   ##
==========================================
+ Coverage   93.53%   94.45%   +0.91%     
==========================================
  Files          32       32              
  Lines        1207     1208       +1     
  Branches      332      333       +1     
==========================================
+ Hits         1129     1141      +12     
+ Misses         74       65       -9     
+ Partials        4        2       -2
Impacted Files Coverage Δ
lib/Server.js 96.92% <100%> (+2.62%) ⬆️
lib/utils/createConfig.js 91.15% <50%> (-0.75%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update fa78347...ae7d586. Read the comment docs.

@hiroppy hiroppy changed the title Make progress option available to API feat(server/client): made progress option available to API Jul 1, 2019
@hiroppy hiroppy merged commit 56274e4 into webpack:master Jul 1, 2019
knagaitsev added a commit to knagaitsev/webpack-dev-server that referenced this pull request Jul 31, 2019
)

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

* test(client): switched snapshot test to single line confirmation

* refactor(server): removed this.profile

* test(client): fixed progress test css path

* test(client): remove jest timeout

* test(e2e): change how use of progress on client checked

* test(e2e): moved css unlink into afterAll

* test(e2e): use port assigner

* test(client): add full progress snapshot

* test(client): reg exp progress test

* test(client): check end of progress updates in console

* test(client): more generalized test reg exp

* test(client): test to isolate ci problem

* test(client): made custom progress plugin to test

* test(client): new progress plugin multi handler test

* test(client): more testing to identify progress problem

* test(client): test with 1 progress plugin

* test(client): new console.log to test sending of data

* feat(server): re-add two progress plugins

* test(client): revert to original changes

* test(progress): added progress and profile option tests

* fix(test): fix profile test port map
@knagaitsev knagaitsev added gsoc Google Summer of Code scope: cli labels Aug 13, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
gsoc Google Summer of Code scope: cli
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants