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

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

Merged
merged 7 commits into from
May 28, 2019
Merged
Show file tree
Hide file tree
Changes from 3 commits
Commits
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
48 changes: 9 additions & 39 deletions lib/Server.js
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,6 @@ const url = require('url');
const http = require('http');
const https = require('https');
const ip = require('ip');
const sockjs = require('sockjs');
const semver = require('semver');
const killable = require('killable');
const chokidar = require('chokidar');
Expand All @@ -32,26 +31,7 @@ const createDomain = require('./utils/createDomain');
const runBonjour = require('./utils/runBonjour');
const routes = require('./utils/routes');
const schema = require('./options.json');

// Workaround for sockjs@~0.3.19
// sockjs will remove Origin header, however Origin header is required for checking host.
// See https://github.com/webpack/webpack-dev-server/issues/1604 for more information
{
// eslint-disable-next-line global-require
const SockjsSession = require('sockjs/lib/transport').Session;
const decorateConnection = SockjsSession.prototype.decorateConnection;
SockjsSession.prototype.decorateConnection = function(req) {
decorateConnection.call(this, req);
const connection = this.connection;
if (
connection.headers &&
!('origin' in connection.headers) &&
'origin' in req.headers
) {
connection.headers.origin = req.headers.origin;
}
};
}
const SockJSServer = require('./servers/sockjsServer');

// Workaround for node ^8.6.0, ^9.0.0
// DEFAULT_ECDH_CURVE is default to prime256v1 in these version
Expand Down Expand Up @@ -674,20 +654,14 @@ class Server {
}

createSocketServer() {
const socket = sockjs.createServer({
// Use provided up-to-date sockjs-client
sockjs_url: '/__webpack_dev_server__/sockjs.bundle.js',
// Limit useless logs
log: (severity, line) => {
if (severity === 'error') {
this.log.error(line);
} else {
this.log.debug(line);
}
},
this.socketServer = new SockJSServer({
error: this.log.error.bind(this),
debug: this.log.debug.bind(this),
server: this.listeningApp,
path: this.sockPath,
Copy link
Member

Choose a reason for hiding this comment

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

Better always pass all options (this.options)

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 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?

Copy link
Member

Choose a reason for hiding this comment

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

@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.

});

socket.on('connection', (connection) => {
this.socketServer.onConnection((connection) => {
if (!connection) {
return;
}
Expand Down Expand Up @@ -739,10 +713,6 @@ class Server {

this._sendStats([connection], this.getStats(this._stats), true);
});

socket.installHandlers(this.listeningApp, {
prefix: this.sockPath,
});
}

listen(port, hostname, fn) {
Expand All @@ -765,7 +735,7 @@ class Server {

close(cb) {
this.sockets.forEach((socket) => {
socket.close();
this.socketServer.close(socket);
});

this.sockets = [];
Expand Down Expand Up @@ -926,7 +896,7 @@ class Server {
// eslint-disable-next-line
sockWrite(sockets, type, data) {
sockets.forEach((socket) => {
socket.write(JSON.stringify({ type, data }));
this.socketServer.send(socket, JSON.stringify({ type, data }));
});
}

Expand Down
5 changes: 5 additions & 0 deletions lib/servers/baseServer.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
'use strict';
Copy link
Member

Choose a reason for hiding this comment

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

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


// base class that users should extend if they are making their own
// server implementation
module.exports = class BaseServer {};
64 changes: 64 additions & 0 deletions lib/servers/sockjsServer.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,64 @@
'use strict';
Copy link
Member

Choose a reason for hiding this comment

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

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


/* eslint-disable
class-methods-use-this,
func-names
*/
const sockjs = require('sockjs');
const BaseServer = require('./baseServer');

// Workaround for sockjs@~0.3.19
// sockjs will remove Origin header, however Origin header is required for checking host.
// See https://github.com/webpack/webpack-dev-server/issues/1604 for more information
{
// eslint-disable-next-line global-require
const SockjsSession = require('sockjs/lib/transport').Session;
const decorateConnection = SockjsSession.prototype.decorateConnection;
SockjsSession.prototype.decorateConnection = function(req) {
decorateConnection.call(this, req);
const connection = this.connection;
if (
connection.headers &&
!('origin' in connection.headers) &&
'origin' in req.headers
) {
connection.headers.origin = req.headers.origin;
}
};
}

module.exports = class SockJSServer extends BaseServer {
// options has: error (function), debug (function), server (http/s server), path (string)
constructor(options) {
super();
this.socket = sockjs.createServer({
// Use provided up-to-date sockjs-client
sockjs_url: '/__webpack_dev_server__/sockjs.bundle.js',
// Limit useless logs
log: (severity, line) => {
if (severity === 'error') {
options.error(line);
} else {
options.debug(line);
}
},
});

this.socket.installHandlers(options.server, {
prefix: options.path,
});
}

send(connection, message) {
connection.write(message);
}

close(connection) {
connection.close();
}

// f should return the resulting connection
onConnection(f) {
this.socket.on('connection', f);
}
};
8 changes: 8 additions & 0 deletions lib/servers/wsServer.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
'use strict';
Copy link
Member

Choose a reason for hiding this comment

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

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


// const ws = require('ws');
const BaseServer = require('./baseServer');

// ws server implementation under construction
// will need changes in the client as well to function
module.exports = class WSServer extends BaseServer {};
29 changes: 8 additions & 21 deletions package-lock.json

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.