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.

Sign up for GitHub

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

Jump to bottom

feat(server): check that serverMode implementation is correct #2051

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
Open

feat(server): check that serverMode implementation is correct #2051

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
62b5ba5
feat(server): check for serverMode implementation problems
knagaitsev Jun 20, 2019
557f7e8
test(server): remove extends BaseServer requirement
knagaitsev Jun 23, 2019
2b10ece
feat(server): test socket server implementation with schema utils
knagaitsev Jun 25, 2019
cf6e74a
fix(server): allow any properties for server impl schema
knagaitsev Jun 25, 2019
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
27 changes: 27 additions & 0 deletions lib/schemas/utils/socketServerImplementationPrototype.json
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,27 @@
{
"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", "send", "close", "onConnection"],
"additionalProperties": false
}
Copy link
Member

@alexander-akait alexander-akait Jun 25, 2019 •
edited
Loading

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 •
edited
Loading

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 •
edited
Loading

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

40 changes: 4 additions & 36 deletions lib/utils/getSocketServerImplementation.js
View file
Open in desktop
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,42 +38,7 @@ function getSocketServerImplementation(options) {
);
}

if (
!ServerImplementation.prototype.constructor ||
ServerImplementation.prototype.constructor.length < 1
) {
throw new Error(
'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')"
);
}

if (
!ServerImplementation.prototype.send ||
ServerImplementation.prototype.send.length < 2
) {
throw new Error(
'serverMode must have a send(connection, message) method that sends the message string to the provided client connection object'
);
}

if (
!ServerImplementation.prototype.close ||
ServerImplementation.prototype.close.length < 1
) {
throw new Error(
'serverMode must have a close(connection) method that closes the provided client connection object'
);
}

if (
!ServerImplementation.prototype.onConnection ||
ServerImplementation.prototype.onConnection.length < 1
) {
throw new Error(
'serverMode must have a onConnection(f) method that calls f(connection) whenever a new client connection is made'
);
}
validateOptions(schema, ServerImplementation.prototype, 'webpack Dev Server');

return ServerImplementation;
}
Expand Down
37 changes: 37 additions & 0 deletions test/server/utils/__snapshots__/getSocketServerImplementation.test.js.snap
View file
Open in desktop
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 should have required property 'close'
options should have required property 'onConnection'
]
`;

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

options should have required property 'constructor'
options should have required property 'send'
options should have required property 'close'
options should have required property 'onConnection'
]
`;

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

options should have required property 'onConnection'
]
`;

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

options should have required property 'send'
options should have required property 'close'
options should have required property 'onConnection'
]
`;
105 changes: 45 additions & 60 deletions test/server/utils/getSocketServerImplementation.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,75 +43,50 @@ describe('getSocketServerImplementation util', () => {
expect(result).toEqual(SockJSServer);
});

it('should throw with serverMode (bad path)', () => {
expect(() => {
getSocketServerImplementation({
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',
});
}).toThrow(/serverMode must be a string denoting a default implementation/);
});

it('should throw with serverMode (incorrect constructor)', () => {
expect(() => {
getSocketServerImplementation({
serverMode: class ServerImplementation extends BaseServer {
constructor() {}
},
});
}).toThrow(
"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')"
);
});

it('should throw with serverMode (no send method)', () => {
expect(() => {
getSocketServerImplementation({
serverMode: class ServerImplementation extends BaseServer {
constructor(server) {
super(server);
}
},
});
}).toThrow(
'serverMode must have a send(connection, message) method that sends the message string to the provided client connection object'
);
});

it('should throw with serverMode (incorrect send parameters)', () => {
expect(() => {
getSocketServerImplementation({
},
},
{
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);
}

send(connection) {}
},
});
}).toThrow(
'serverMode must have a send(connection, message) method that sends the message string to the provided client connection object'
);
});

it('should throw with serverMode (no close method)', () => {
expect(() => {
getSocketServerImplementation({
},
},
{
title: 'should throw with serverMode (no close, onConnection methods)',
config: {
serverMode: class ServerImplementation extends BaseServer {
constructor(server) {
super(server);
}

send(connection, message) {}
},
});
}).toThrow(
'serverMode must have a close(connection) method that closes the provided client connection object'
);
});

it('should throw with serverMode (no onConnection method)', () => {
expect(() => {
getSocketServerImplementation({
},
},
{
title: 'should throw with serverMode (no onConnection method)',
config: {
serverMode: class ServerImplementation extends BaseServer {
constructor(server) {
super(server);
Expand All @@ -121,9 +96,19 @@ describe('getSocketServerImplementation util', () => {

close(connection) {}
},
});
}).toThrow(
'serverMode must have a onConnection(f) method that calls f(connection) whenever a new client connection is made'
);
},
},
];

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