feat(client): add clientMode option #1977

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

lib/options.json

@@ -390,6 +393,7 @@
      "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

lib/Server.js

+      );
+    }
+
+    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

test/e2e/ClientMode.test.js

+  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

test/e2e/__snapshots__/ClientMode.test.js.snap

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


Why isn't onClose called?

@hiroppy testServer.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 db1067c June 6, 2019 17:10

alexander-akait requested changes Jun 6, 2019

View reviewed changes

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

alexander-akait previously approved these changes Jun 10, 2019

View reviewed changes

hiroppy requested changes Jun 10, 2019

View reviewed changes

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

knagaitsev force-pushed the clientMode-option branch from ee8a4b2 to a80df52 Compare 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 ab19944 June 13, 2019 16:21

knagaitsev force-pushed the clientMode-option branch 3 times, most recently from 302b26a to bdbe023 Compare 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 5745e7d Compare 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 c4453fd June 26, 2019 17:43

hiroppy approved these changes Jun 26, 2019

View reviewed changes

alexander-akait merged commit 6141018 into webpack:master Jun 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!

feat(client): add clientMode option #1977

feat(client): add clientMode option #1977

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!

codecov bot 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!

Uh oh!

codecov bot commented Jun 6, 2019 •

edited

Loading

alexander-akait left a comment •

edited

Loading

knagaitsev commented Jun 26, 2019 •

edited

Loading