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): add serverMode option #1937

Merged
merged 7 commits into from
Jun 1, 2019
Merged
Show file tree
Hide file tree
Changes from 2 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
43 changes: 41 additions & 2 deletions lib/Server.js
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,6 @@ const createDomain = require('./utils/createDomain');
const runBonjour = require('./utils/runBonjour');
const routes = require('./utils/routes');
const schema = require('./options.json');
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 @@ -67,6 +66,45 @@ class Server {

this.log = _log || createLogger(options);

if (this.options.serverMode !== undefined) {
this.log.warn(
'serverMode is an experimental option, meaning its usage could potentially change without warning'
);
}

this.options.serverMode = this.options.serverMode || 'sockjs';
let ServerImplementation;
let serverImplFound = true;
if (typeof this.options.serverMode === 'string') {
// could be 'sockjs', in the future 'ws', or a path that should be required
if (this.options.serverMode === 'sockjs') {
// eslint-disable-next-line global-require
ServerImplementation = require('./servers/SockJSServer');
} else {
try {
// eslint-disable-next-line global-require, import/no-dynamic-require
ServerImplementation = require(this.options.serverMode);
} catch (e) {
serverImplFound = false;
}
}
} else if (typeof this.options.serverMode === 'function') {
// potentially do more checks here to confirm that the user implemented this properlly
// since errors could be difficult to understand
ServerImplementation = this.options.serverMode;
} else {
serverImplFound = false;
}

if (!serverImplFound) {
throw new Error(
"serverMode must be a string denoting a default implementation (eg. 'sockjs'), a full path to " +
'a JS file which exports a class extending BaseServer (webpack-dev-server/lib/servers/BaseServer) ' +
'via require.resolve(...), or the class itself which extends BaseServer'
);
}
this.ServerImplementation = ServerImplementation;
Copy link
Member

Choose a reason for hiding this comment

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

  1. Let's move this to util getSocketServerImplementation
  2. Don't think we need function
  3. Let's rewrite this on switch

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 function option is good if the user wants to supply an implementation, but their file is exporting multiple things, for example. So like if they want to do:

serverMode: require('./my-implementation').Server

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 require.resolve('./my-implementation/Server')
/cc @hiroppy

Copy link
Member

Choose a reason for hiding this comment

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

I'm fine either way:) Currently, it was fixed as require.resolve.


this.originalStats =
this.options.stats && Object.keys(this.options.stats).length
? this.options.stats
Expand Down Expand Up @@ -655,7 +693,8 @@ class Server {
}

createSocketServer() {
this.socketServer = new SockJSServer(this);
const ServerImplementation = this.ServerImplementation;
this.socketServer = new ServerImplementation(this);

this.socketServer.onConnection((connection) => {
if (!connection) {
Expand Down
11 changes: 11 additions & 0 deletions lib/options.json
Original file line number Diff line number Diff line change
Expand Up @@ -297,6 +297,16 @@
"serveIndex": {
"type": "boolean"
},
"serverMode": {
"anyOf": [
{
"type": "string"
},
{
"instanceof": "Function"
}
]
},
"serverSideRender": {
"type": "boolean"
},
Expand Down Expand Up @@ -420,6 +430,7 @@
"reporter": "should be {Function} (https://github.com/webpack/webpack-dev-middleware#reporter)",
"requestCert": "should be {Boolean}",
"serveIndex": "should be {Boolean} (https://webpack.js.org/configuration/dev-server/#devserverserveindex)",
"serverMode": "should be {String|Function} (https://webpack.js.org/configuration/dev-server/#devserverservermode-)",
"serverSideRender": "should be {Boolean} (https://github.com/webpack/webpack-dev-middleware#serversiderender)",
"setup": "should be {Function} (https://webpack.js.org/configuration/dev-server/#devserversetup)",
"sockHost": "should be {String|Null} (https://webpack.js.org/configuration/dev-server/#devserversockhost)",
Expand Down
133 changes: 133 additions & 0 deletions test/ServerMode.test.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,133 @@
'use strict';

/* eslint-disable
class-methods-use-this
*/
const request = require('supertest');
const sockjs = require('sockjs');
const SockJSServer = require('../lib/servers/SockJSServer');
const config = require('./fixtures/simple-config/webpack.config');
const testServer = require('./helpers/test-server');
const BaseServer = require('./../lib/servers/BaseServer');

describe('serverMode', () => {
let server;
let req;

afterEach((done) => {
testServer.close(done);
req = null;
server = null;
});
describe("supplying 'sockjs' as a string", () => {
beforeEach((done) => {
server = testServer.start(
config,
{
serverMode: 'sockjs',
},
done
);
req = request('http://localhost:8080');
});

it('sockjs path responds with a 200', (done) => {
req.get('/sockjs-node').expect(200, done);
});
});

describe('supplying path to sockjs implementation', () => {
beforeEach((done) => {
server = testServer.start(
config,
{
serverMode: require.resolve('../lib/servers/SockJSServer'),
},
done
);
req = request('http://localhost:8080');
});

it('sockjs path responds with a 200', (done) => {
req.get('/sockjs-node').expect(200, done);
});
});

describe('supplying sockjs implementation class', () => {
beforeEach((done) => {
server = testServer.start(
config,
{
serverMode: SockJSServer,
},
done
);
req = request('http://localhost:8080');
});

it('sockjs path responds with a 200', (done) => {
req.get('/sockjs-node').expect(200, done);
});
});

describe('custom sockjs implementation', () => {
it('uses supplied server implementation', (done) => {
server = testServer.start(
config,
{
sockPath: '/foo/test/bar/',
serverMode: class MySockJSServer extends BaseServer {
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 being able to pass in class also makes testing easier

constructor(serv) {
super(serv);
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') {
this.server.log.error(line);
} else {
this.server.log.debug(line);
}
},
});

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

expect(server.options.sockPath).toEqual('/foo/test/bar/');
}

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

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

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

describe('supplying nonexistent path', () => {
it('should throw an error', () => {
expect(() => {
server = testServer.start(
config,
{
serverMode: '/bad/path/to/implementation',
},
() => {}
);
}).toThrow(/serverMode must be a string/);
});
});
});
10 changes: 10 additions & 0 deletions test/options.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ const webpack = require('webpack');
const { createFsFromVolume, Volume } = require('memfs');
const Server = require('../lib/Server');
const options = require('../lib/options.json');
const SockJSServer = require('../lib/servers/SockJSServer');
const config = require('./fixtures/simple-config/webpack.config');

describe('options', () => {
Expand Down Expand Up @@ -340,6 +341,15 @@ describe('options', () => {
success: [true],
failure: [''],
},
serverMode: {
success: [
'',
Copy link
Member

Choose a reason for hiding this comment

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

'' should be failure

'sockjs',
require.resolve('../lib/servers/SockJSServer'),
SockJSServer,
],
failure: [false],
},
serverSideRender: {
success: [true],
failure: [''],
Expand Down