feat(client): add clientMode option by knagaitsev · Pull Request #1977 · webpack/webpack-dev-server

knagaitsev · 2019-06-06T04:38:39Z

For Bugs and Features; did you add new tests?

Yes

Motivation / Use-Case

This option makes it possible to provide a custom client socket implementation.

This mimics the serverMode implementation with one exception: clientMode does not accept a function (specifically, a class, as serverMode does). The reason for this is that it is not possible (or just pointlessly complex and impractical) to make a class within an executing server-side script, then pass it along to the client. There must be a path to the file that module.exports the client implementation.

Since the only thing we really need to pass to the client is the path to the implementation, I made the helper getSocketClientPath. There is no reason for a helper to be returning the whole ClientImplementation class, since it is not actually used server-side.

Breaking Changes

None

Additional Info

One consideration:
Should more error handling be added to confirm that the serverMode/clientMode are implemented correctly? For example, there could be some functionality in the Dev Server that tests the contents of ClientImplementation.protoype, to see if all the required functions are implemented.

codecov · 2019-06-06T04:44:12Z

Codecov Report

Merging #1977 into master will increase coverage by 0.03%.
The diff coverage is 90.47%.

@@ Coverage Diff @@## master #1977 +/- ##
==========================================
+ Coverage 92.71% 92.75% +0.03% 
==========================================
Files 31 32 +1 Lines 1181 1201 +20 Branches 329 335 +6 ==========================================
+ Hits 1095 1114 +19 - Misses 82 83 +1 
Partials 4 4

Impacted Files	Coverage Δ
lib/Server.js	`92.77% <100%> (+0.26%)`	⬆️
lib/utils/updateCompiler.js	`100% <100%> (ø)`	⬆️
lib/utils/getSocketClientPath.js	`87.5% <87.5%> (ø)`

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 1437625...c4453fd. Read the comment docs.

alexander-akait

Good work, one note.
/cc @hiroppy

alexander-akait · 2019-06-06T11:27:41Z

 "ca": "should be {String|Buffer}",
 "cert": "should be {String|Buffer}",
 "clientLogLevel": "should be {String} and equal to one of the allowed values\n\n [ 'info', 'warn', 'error', 'debug', 'trace', 'silent' ]\n\n (https://webpack.js.org/configuration/dev-server/#devserverclientloglevel)",
+ "clientMode": "should be {String} (https://webpack.js.org/configuration/dev-server/#devserverclientmode-)",


extra - at end of line

Do we need to modify this? L437 was fixed it.

Do we need to modify this? L437 was fixed it.
Yes, I mistakenly did not remove that -, but they should be removed in both places.

hiroppy · 2019-06-06T12:01:46Z

CI fails.

options validation clientMode

Error: expect(received).toBeInstanceOf(expected)
Expected constructor: ValidationError
Received constructor: JestAssertionError

knagaitsev · 2019-06-06T16:46:08Z

getSocketClientPath(options) is only called if inline is enabled, so the clientMode option is only validated by it when inline is enabled. Should I keep it as is, move it outside the inline check so that the clientMode option gets validated anyway, or throw an error when clientMode is provided without inline mode enabled?

knagaitsev · 2019-06-06T17:15:28Z

I'm now wondering why this empty string is passing this test: https://github.com/webpack/webpack-dev-server/blob/master/test/options.test.js#L350. Because that should not be throwing a ValidationError, it should be throwing an error later on. Problem with options.test.js?

alexander-akait · 2019-06-06T22:04:33Z

/cc @Loonride need rebase

alexander-akait

One note

alexander-akait · 2019-06-06T22:05:14Z

+ );
+ }
+
+ updateCompiler(this.compiler, this.options);


Why we move this?

@evilebottnawi I reorganized it to have the default setting for serverMode and clientMode together above that. updateCompiler uses this.options.clientMode, so the default setting for it must be set before updateCompiler is called. It can be switched back, I just thought it would be nice to have default setting for serverMode and clientMode right next to one another

alexander-akait

/cc @hiroppy

hiroppy · 2019-06-10T14:28:19Z

+ describe('sockjs', () => {
+ beforeAll((done) => {
+ const options = {
+ port: 9000,


Sorry but I change tests system. See #2005.
And I'll merge #2007 asap. Please change 🙏

@hiroppy done

hiroppy · 2019-06-11T07:54:02Z

+ "liveReload",
+ "[WDS] Live Reloading enabled.",
+ "hash",
+ "ok",


Why isn't onClose called?

@hiroppytestServer.close happens after the snapshot is tested, so the client is not closed before that. I could maybe rearrange that to test onClose as well.

alexander-akait · 2019-06-13T11:15:57Z

/cc @Loonride need rebase, great job!

knagaitsev · 2019-06-26T17:10:30Z

@evilebottnawi @hiroppy Can I get a final review for this?

alexander-akait

/cc @hiroppy

knagaitsev · 2019-06-27T16:20:19Z

/cc @evilebottnawi

knagaitsev requested review from alexander-akait and hiroppy as code owners June 6, 2019 04:38

alexander-akait requested changes Jun 6, 2019
View reviewed changes

hiroppy previously approved these changes Jun 6, 2019
View reviewed changes

knagaitsev dismissed hiroppy’s stale review via db1067cJune 6, 2019 17:10

alexander-akait requested changes Jun 6, 2019
View reviewed changes

knagaitsev force-pushed the clientMode-option branch from db1067c to ee8a4b2Compare June 8, 2019 20:04

alexander-akait previously approved these changes Jun 10, 2019
View reviewed changes

hiroppy suggested changes Jun 10, 2019
View reviewed changes

knagaitsev dismissed alexander-akait’s stale review via a80df52June 11, 2019 02:38

knagaitsev force-pushed the clientMode-option branch from ee8a4b2 to a80df52Compare June 11, 2019 02:38

knagaitsev closed this Jun 11, 2019

knagaitsev reopened this Jun 11, 2019

hiroppy reviewed Jun 11, 2019
View reviewed changes

hiroppy previously approved these changes Jun 11, 2019
View reviewed changes

wizardofhogwarts previously approved these changes Jun 13, 2019
View reviewed changes

alexander-akait previously approved these changes Jun 13, 2019
View reviewed changes

knagaitsev dismissed stale reviews from alexander-akait, wizardofhogwarts, and hiroppy via ab19944June 13, 2019 16:21

knagaitsev force-pushed the clientMode-option branch 3 times, most recently from 302b26a to bdbe023Compare June 20, 2019 02:02

feat(client): add clientMode option
3833725

knagaitsev added 6 commits June 26, 2019 12:04

test(options): pass options test
d784686

fix(client): fix paths to clients after rebase
3ec66a6

test(client): update port method
5ef83e5

test(client): moved new port mapping to end of list
ed53db6

test(client): test close, update log snapshots
e205363

test(client): close browser before running test
5745e7d

knagaitsev force-pushed the clientMode-option branch from bdbe023 to 5745e7dCompare June 26, 2019 17:06

alexander-akait previously approved these changes Jun 26, 2019
View reviewed changes

fix(options): removed incorrect dash at clientMode link
c4453fd

knagaitsev dismissed alexander-akait’s stale review via c4453fdJune 26, 2019 17:43

hiroppy approved these changes Jun 26, 2019
View reviewed changes

alexander-akait merged commit 6141018 into webpack:masterJun 27, 2019

knagaitsev added a commit to knagaitsev/webpack-dev-server that referenced this pull request Jul 31, 2019
feat(client): add clientMode option (webpack#1977)
e8a537a

knagaitsev added gsoc Google Summer of Code scope: ws(s) labels Aug 13, 2019

Uh oh!

Uh oh!

Conversation

knagaitsev commented Jun 6, 2019

For Bugs and Features; did you add new tests?

Motivation / Use-Case

Breaking Changes

Additional Info

Uh oh!

codecovBot commented Jun 6, 2019• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

alexander-akait left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hiroppy commented Jun 6, 2019

Uh oh!

knagaitsev commented Jun 6, 2019

Uh oh!

knagaitsev commented Jun 6, 2019

Uh oh!

alexander-akait commented Jun 6, 2019

Uh oh!

alexander-akait left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alexander-akait left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alexander-akait commented Jun 13, 2019

Uh oh!

knagaitsev commented Jun 26, 2019• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

alexander-akait left a comment

Choose a reason for hiding this comment

Uh oh!

knagaitsev commented Jun 27, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

codecovBot commented Jun 6, 2019•
edited
Loading

alexander-akait left a comment •
edited
Loading

knagaitsev commented Jun 26, 2019•
edited
Loading