Encapsulate sockjs, introduce ws and build base for users to make own server implementation #1904

knagaitsev · 2019-05-17T20:41:28Z

For Bugs and Features; did you add new tests?

No

Motivation / Use-Case

Refactored to encapsulate sockjs better so that it is easier to implement ws or other user defined server implementations.

Issue: #1860

Breaking Changes

I am unsure if moving sockjs out breaks this: https://github.com/webpack/webpack-dev-server/blob/master/lib/Server.js#L41

Otherwise there should be no breaking changes.

Additional Info

Tests were failing locally, I think it is related to a snapshots though: #1889

codecov · 2019-05-17T20:45:13Z

Codecov Report

Merging #1904 into master will decrease coverage by 1.22%.
The diff coverage is 60%.

@@            Coverage Diff             @@
##           master    #1904      +/-   ##
==========================================
- Coverage      88%   86.77%   -1.23%     
==========================================
  Files          16       20       +4     
  Lines         817      862      +45     
  Branches      260      265       +5     
==========================================
+ Hits          719      748      +29     
- Misses         84       99      +15     
- Partials       14       15       +1

Impacted Files	Coverage Δ
lib/servers/baseServer.js	`100% <100%> (ø)`
lib/servers/BaseServer.js	`100% <100%> (ø)`
lib/servers/SockJSServer.js	`55.55% <55.55%> (ø)`
lib/servers/sockjsServer.js	`55.55% <55.55%> (ø)`
lib/Server.js	`87.27% <60%> (+1.31%)`	⬆️

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 87cfaa7...bee6f83. Read the comment docs.

alexander-akait · 2019-05-23T10:30:55Z

lib/Server.js

@@ -32,6 +31,7 @@ const createDomain = require('./utils/createDomain');
 const runBonjour = require('./utils/runBonjour');
 const routes = require('./utils/routes');
 const schema = require('./options.json');
+const SockjsServer = require('./utils/server/sockjsServer');

 // Workaround for sockjs@~0.3.19


Let's move workaround to sockjsServer

You mean this workaround? https://github.com/webpack/webpack-dev-server/blob/master/lib/Server.js#L41

alexander-akait · 2019-05-23T10:31:28Z

lib/Server.js


-    socket.on('connection', (connection) => {
+    socket.onConnection((connection) => {


Better use this.sockerServer everywhere

alexander-akait · 2019-05-23T10:32:17Z

lib/utils/server/sockjsServer.js

+  onConnection(f) {
+    this.socket.on('connection', f);
+  }
+};


Let's move this to lib/servers/sockjsServer.js, it is not util

alexander-akait · 2019-05-23T10:32:56Z

package.json

@@ -62,6 +62,7 @@
    "url": "^0.11.0",
    "webpack-dev-middleware": "^3.7.0",
    "webpack-log": "^2.0.0",
+    "ws": "^6.2.1",
    "yargs": "12.0.5"
  },


No need this right now, it is increase package size

alexander-akait · 2019-05-23T10:33:54Z

lib/Server.js

@@ -32,6 +31,7 @@ const createDomain = require('./utils/createDomain');
 const runBonjour = require('./utils/runBonjour');
 const routes = require('./utils/routes');
 const schema = require('./options.json');
+const SockjsServer = require('./utils/server/sockjsServer');


rename sockjsServer to SockJSServer because it is class

alexander-akait · 2019-05-23T10:34:36Z

lib/Server.js

+      error: this.log.error.bind(this),
+      debug: this.log.debug.bind(this),
+      server: this.listeningApp,
+      path: this.sockPath,


Better always pass all options (this.options)

@evilebottnawi Are you suggesting, in addition to what is already there, options: this.options? Because this.listeningApp is not part of the options, along with the error, debug callbacks.

Users may be creating server implementations of their own, so they would be interacting with the options passed in. Do you think it's a good idea to expose all the options to the user?

@Loonride

Do you think it's a good idea to expose all the options to the user?

Yes, we because behavior can be changed based on options (not only) in theory.

Let's do it like:
new SockJSServer(this);

Custom implementation should have access to compiler, application, server, options and devServer.

Also it is not available for developers right now, so we can change this in future.

alexander-akait

Some notes, anyway good idea and good solution

alexander-akait

One note

alexander-akait · 2019-05-24T10:40:57Z

/cc @hiroppy

hiroppy · 2019-05-24T15:43:26Z

lib/servers/sockjsServer.js

@@ -0,0 +1,64 @@
+'use strict';


The file name should be changed from sockjsServer.js to SockJSServer.

hiroppy · 2019-05-24T15:43:52Z

lib/servers/wsServer.js

@@ -0,0 +1,8 @@
+'use strict';


The file name should be changed from wsServer.js to WsServer.

hiroppy · 2019-05-24T15:45:17Z

lib/servers/baseServer.js

@@ -0,0 +1,5 @@
+'use strict';


The file name should be changed from baseServer.js to BaseServer.

alexander-akait · 2019-05-27T10:26:25Z

lib/servers/wsServer.js


 // ws server implementation under construction
 // will need changes in the client as well to function
-module.exports = class WSServer extends BaseServer {};
+module.exports = class WsServer extends BaseServer {};


/cc @hiroppy What do you think is better:

WebsocketServer

WSServer

WsServer

I like WebsocketServer. For example, the plugin name of wasm(WebAssemblyPlugin) which webpack has is not omitted. But this example is webpack, not webpack-dev-server. In any case, I think that it is easier to understand if it is not omitted.

👍 let's use WebsocketServer

/cc @Loonride

alexander-akait

Looks good, one questions

/cc @hiroppy

alexander-akait

/cc @hiroppy

knagaitsev · 2019-05-27T19:23:55Z

@evilebottnawi, @hulkish says just to continue work in this pr, are you fine with that or do you want to get the base changes merged first?

hiroppy · 2019-05-27T19:40:55Z

Please don't continue to work in this pr. Because don't give PR more than one content, as it will be difficult to review.

hulkish · 2019-05-27T19:45:24Z

@hiroppy

Plese don't continue to work in this pr. Because don't give PR more than one content, as it will be difficult to review.

I disagree, this is all part of the same gsoc effort. What things do u think deviate enough to be considered separate?

hulkish · 2019-05-27T19:47:19Z

@Loonride please add tests which validate support for clientMode & serverMode (as we discussed on slack)

hiroppy · 2019-05-27T19:50:41Z

We have next branch for the major version. If this pr is changed to next branch, this problem will be resolved because it is not used now? Or do you want a branch dedicated to GSoC? I oppose continuing development on this branch. You should create a dedicated branch and submit PR there. Continued development on this branch will increase the cost of the reviewer.

hiroppy · 2019-05-27T19:59:57Z

If you want to need gsoc branch, I'll create it.

hulkish · 2019-05-27T20:04:52Z

@hiroppy How about:

create next branch, if not exists already.
point the target branch for this pr to next
merge this pr to next once code review passes
go from there?

knagaitsev · 2019-05-27T20:06:26Z

I like the idea of having gsoc / next branch. No chance of adding a breaking change in master as I implement things, and also I will be more efficient if I can do more work per PR.

hulkish · 2019-05-27T20:09:41Z

@Loonride i think that may be beside the point - your code will be merged once stable (with tests)

then, the next branch will act as the living alpha or beta, until the collection of changes have all made it in and are stable

hiroppy · 2019-05-27T20:12:43Z

this is all part of the same gsoc effort.

I agree with this idea. I think it is a good idea to put them into one branch so that you can easily visualize the results.

I like the idea of creating gsoc branch. because maybe I'll use next branch for webpack5. Also, don't forget that you need to periodically rebase from master branch.

knagaitsev · 2019-05-28T03:09:31Z

Should I add tests on the new files to pass codecov?

knagaitsev added 2 commits May 17, 2019 01:36

refactor(server): encapsulated sockjs

5a6b2a4

refactor(server): added some comments, removed commented out code

2f22843

knagaitsev requested review from alexander-akait and hiroppy as code owners May 17, 2019 20:41

alexander-akait reviewed May 23, 2019

View reviewed changes

alexander-akait requested changes May 23, 2019

View reviewed changes

refactor(server): moved server implementations, fix naming

62cc34c

alexander-akait reviewed May 24, 2019

View reviewed changes

hiroppy requested changes May 24, 2019

View reviewed changes

knagaitsev added 2 commits May 24, 2019 15:09

refactor(server): rename files, pass in full server object

f915ce5

refactor(server): fixed capitalization

8f6bd69

alexander-akait reviewed May 27, 2019

View reviewed changes

alexander-akait previously approved these changes May 27, 2019

View reviewed changes

refactor(server): renamed ws server

11b6522

knagaitsev dismissed alexander-akait’s stale review via 11b6522 May 27, 2019 17:42

alexander-akait approved these changes May 27, 2019

View reviewed changes

hiroppy approved these changes May 27, 2019

View reviewed changes

Merge branch 'master' into clientMode

bee6f83

alexander-akait merged commit d7aaef6 into webpack:master May 28, 2019

knagaitsev mentioned this pull request May 28, 2019

Delete incorrectly capitalized server files #1931

Merged

6 tasks

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Encapsulate sockjs, introduce ws and build base for users to make own server implementation #1904

Encapsulate sockjs, introduce ws and build base for users to make own server implementation #1904

knagaitsev commented May 17, 2019

codecov bot commented May 17, 2019 •

edited

Loading

alexander-akait May 23, 2019

knagaitsev May 23, 2019

alexander-akait May 23, 2019

alexander-akait May 23, 2019 •

edited

Loading

alexander-akait May 23, 2019 •

edited

Loading

alexander-akait May 23, 2019

alexander-akait May 23, 2019

alexander-akait May 23, 2019

knagaitsev May 23, 2019

alexander-akait May 24, 2019

alexander-akait left a comment

alexander-akait left a comment

alexander-akait commented May 24, 2019

hiroppy May 24, 2019

hiroppy May 24, 2019

hiroppy May 24, 2019

alexander-akait May 27, 2019

hiroppy May 27, 2019

alexander-akait May 27, 2019

alexander-akait left a comment

alexander-akait left a comment •

edited

Loading

knagaitsev commented May 27, 2019

hiroppy commented May 27, 2019 •

edited

Loading

hulkish commented May 27, 2019

hulkish commented May 27, 2019

hiroppy commented May 27, 2019 •

edited

Loading

hiroppy commented May 27, 2019

hulkish commented May 27, 2019

knagaitsev commented May 27, 2019

hulkish commented May 27, 2019

hiroppy commented May 27, 2019 •

edited

Loading

knagaitsev commented May 28, 2019


		socket.on('connection', (connection) => {
		socket.onConnection((connection) => {

Encapsulate sockjs, introduce ws and build base for users to make own server implementation #1904

Encapsulate sockjs, introduce ws and build base for users to make own server implementation #1904

Conversation

knagaitsev commented May 17, 2019

For Bugs and Features; did you add new tests?

Motivation / Use-Case

Breaking Changes

Additional Info

codecov bot commented May 17, 2019 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alexander-akait May 23, 2019 • edited Loading

Choose a reason for hiding this comment

alexander-akait May 23, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alexander-akait left a comment

Choose a reason for hiding this comment

alexander-akait left a comment

Choose a reason for hiding this comment

alexander-akait commented May 24, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alexander-akait left a comment

Choose a reason for hiding this comment

alexander-akait left a comment • edited Loading

Choose a reason for hiding this comment

knagaitsev commented May 27, 2019

hiroppy commented May 27, 2019 • edited Loading

hulkish commented May 27, 2019

hulkish commented May 27, 2019

hiroppy commented May 27, 2019 • edited Loading

hiroppy commented May 27, 2019

hulkish commented May 27, 2019

knagaitsev commented May 27, 2019

hulkish commented May 27, 2019

hiroppy commented May 27, 2019 • edited Loading

knagaitsev commented May 28, 2019

codecov bot commented May 17, 2019 •

edited

Loading

alexander-akait May 23, 2019 •

edited

Loading

alexander-akait May 23, 2019 •

edited

Loading

alexander-akait left a comment •

edited

Loading

hiroppy commented May 27, 2019 •

edited

Loading

hiroppy commented May 27, 2019 •

edited

Loading

hiroppy commented May 27, 2019 •

edited

Loading