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): check that serverMode implementation is correct #2051

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all 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
33 changes: 33 additions & 0 deletions lib/schemas/utils/socketServerImplementationPrototype.json
Original file line number Diff line number Diff line change
@@ -0,0 +1,33 @@
{
"type": "object",
"properties": {
"constructor": {
"instanceof": "Function"
},
"send": {
"instanceof": "Function"
},
"close": {
"instanceof": "Function"
},
"onConnection": {
"instanceof": "Function"
}
},
"errorMessage": {
"properties": {
"constructor": "- serverMode must have a constructor that takes a single server argument and calls super(server) on the superclass BaseServer, found via require('webpack-dev-server/lib/servers/BaseServer')",
"send": "- serverMode must have a send(connection, message) method that sends the message string to the provided client connection object",
"close": "- serverMode must have a close(connection) method that closes the provided client connection object",
"onConnection": "- serverMode must have a onConnection(f) method that calls f(connection) whenever a new client connection is made"
},
"required": {
"constructor": "- serverMode must have a constructor that takes a single server argument and calls super(server) on the superclass BaseServer, found via require('webpack-dev-server/lib/servers/BaseServer')",
"send": "- serverMode must have a send(connection, message) method that sends the message string to the provided client connection object",
"close": "- serverMode must have a close(connection) method that closes the provided client connection object",
"onConnection": "- serverMode must have a onConnection(f) method that calls f(connection) whenever a new client connection is made"
}
},
"required": ["constructor", "send", "close", "onConnection"],
"additionalProperties": true
}
Copy link
Member

@alexander-akait alexander-akait Jun 25, 2019

Choose a reason for hiding this comment

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

I think we can have one schema (put this inside original schema) for better maintenance

Copy link
Collaborator Author

@knagaitsev knagaitsev Jun 25, 2019

Choose a reason for hiding this comment

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

@evilebottnawi The problem is that I couldn't find a way to test the prototype of a function with schema-utils when it matched "instanceof": "Function". I think it only looks at "properties" if the thing in question matches "type": "object", hence why I pass ServerImplementation.prototype into the schema

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Unless you just mean make options.json into something like:

{
"defaultSchema": { ... },
"socketServerImplementationPrototypeSchema": { ... }
}

But that could be a breaking change if something else is using this schema.

Copy link
Member

@alexander-akait alexander-akait Jun 25, 2019

Choose a reason for hiding this comment

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

hm we can rewrite schema-utils and allow to define own validator (where we can implement any logic), i will take care about this, let's keep this PR open, it is not high priority because we can improve documentation

5 changes: 5 additions & 0 deletions lib/utils/getSocketServerImplementation.js
Original file line number Diff line number Diff line change
@@ -1,5 +1,8 @@
'use strict';

const validateOptions = require('schema-utils');
const schema = require('../schemas/utils/socketServerImplementationPrototype.json');

function getSocketServerImplementation(options) {
let ServerImplementation;
let serverImplFound = true;
Expand Down Expand Up @@ -35,6 +38,8 @@ function getSocketServerImplementation(options) {
);
}

validateOptions(schema, ServerImplementation.prototype, 'webpack Dev Server');

return ServerImplementation;
}

Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,37 @@
// Jest Snapshot v1, https://goo.gl/fbAQLP

exports[`getSocketServerImplementation util should throw with serverMode (bad path) 1`] = `[Error: serverMode must be a string denoting a default implementation (e.g. '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]`;

exports[`getSocketServerImplementation util should throw with serverMode (no close, onConnection methods) 1`] = `
[ValidationError: webpack Dev Server Invalid Options

options - serverMode must have a close(connection) method that closes the provided client connection object
options - serverMode must have a onConnection(f) method that calls f(connection) whenever a new client connection is made
]
`;

exports[`getSocketServerImplementation util should throw with serverMode (no constructor, send, close, onConnection methods) 1`] = `
[ValidationError: webpack Dev Server Invalid Options

options [object Object]
options - serverMode must have a send(connection, message) method that sends the message string to the provided client connection object
options - serverMode must have a close(connection) method that closes the provided client connection object
options - serverMode must have a onConnection(f) method that calls f(connection) whenever a new client connection is made
]
`;

exports[`getSocketServerImplementation util should throw with serverMode (no onConnection method) 1`] = `
[ValidationError: webpack Dev Server Invalid Options

options - serverMode must have a onConnection(f) method that calls f(connection) whenever a new client connection is made
]
`;

exports[`getSocketServerImplementation util should throw with serverMode (no send, close, onConnection methods) 1`] = `
[ValidationError: webpack Dev Server Invalid Options

options - serverMode must have a send(connection, message) method that sends the message string to the provided client connection object
options - serverMode must have a close(connection) method that closes the provided client connection object
options - serverMode must have a onConnection(f) method that calls f(connection) whenever a new client connection is made
]
`;
94 changes: 88 additions & 6 deletions test/server/utils/getSocketServerImplementation.test.js
Original file line number Diff line number Diff line change
@@ -1,10 +1,13 @@
'use strict';

/* eslint-disable constructor-super, no-empty-function, no-useless-constructor, no-unused-vars, class-methods-use-this */

const getSocketServerImplementation = require('../../../lib/utils/getSocketServerImplementation');
const BaseServer = require('../../../lib/servers/BaseServer');
const SockJSServer = require('../../../lib/servers/SockJSServer');

describe('getSocketServerImplementation util', () => {
it("should works with string serverMode ('sockjs')", () => {
it("should work with string serverMode ('sockjs')", () => {
let result;

expect(() => {
Expand All @@ -16,7 +19,7 @@ describe('getSocketServerImplementation util', () => {
expect(result).toEqual(SockJSServer);
});

it('should works with serverMode (SockJSServer class)', () => {
it('should work with serverMode (SockJSServer class)', () => {
let result;

expect(() => {
Expand All @@ -40,11 +43,90 @@ describe('getSocketServerImplementation util', () => {
expect(result).toEqual(SockJSServer);
});

it('should throws with serverMode (bad path)', () => {
it('should work with serverMode (additional class methods)', () => {
let result;

const ExtendedSockJSServer = class ExtendedSockJSServer extends SockJSServer {
myMethod() {
this.test = true;
}
};

expect(() => {
getSocketServerImplementation({
serverMode: '/bad/path/to/implementation',
result = getSocketServerImplementation({
serverMode: ExtendedSockJSServer,
});
}).toThrow(/serverMode must be a string/);
}).not.toThrow();

expect(result).toEqual(ExtendedSockJSServer);
});

const ClassWithoutConstructor = class ClassWithoutConstructor {};
// eslint-disable-next-line no-undefined
ClassWithoutConstructor.prototype.constructor = undefined;

const badSetups = [
{
title: 'should throw with serverMode (bad path)',
config: {
serverMode: '/bad/path/to/implementation',
},
},
{
title:
'should throw with serverMode (no constructor, send, close, onConnection methods)',
config: {
serverMode: ClassWithoutConstructor,
},
},
{
title:
'should throw with serverMode (no send, close, onConnection methods)',
config: {
serverMode: class ServerImplementation extends BaseServer {
constructor(server) {
super(server);
}
},
},
},
{
title: 'should throw with serverMode (no close, onConnection methods)',
config: {
serverMode: class ServerImplementation extends BaseServer {
constructor(server) {
super(server);
}

send(connection, message) {}
},
},
},
{
title: 'should throw with serverMode (no onConnection method)',
config: {
serverMode: class ServerImplementation extends BaseServer {
constructor(server) {
super(server);
}

send(connection, message) {}

close(connection) {}
},
},
},
];

badSetups.forEach((setup) => {
it(setup.title, () => {
let thrown = false;
try {
getSocketServerImplementation(setup.config);
} catch (e) {
thrown = true;
expect(e).toMatchSnapshot();
}
});
});
});