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: add sockPath option (options.sockPath) #1553

Merged
merged 19 commits into from
Jan 30, 2019
Merged
Show file tree
Hide file tree
Changes from 17 commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
4 changes: 2 additions & 2 deletions client-src/default/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@

/* global __resourceQuery WorkerGlobalScope self */
/* eslint prefer-destructuring: off */

const querystring = require('querystring');
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
const querystring = require('querystring');
const qs = require('querystring');

Copy link
Member

Choose a reason for hiding this comment

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

Disagree, no need rewrite full package name to short name.

Copy link
Contributor Author

@trescenzi trescenzi Oct 30, 2018

Choose a reason for hiding this comment

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

qs also conflicts with another variable in the file, and eslint complains about shadowing. That's why it's been renamed from the original PR.

Copy link
Member

Choose a reason for hiding this comment

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

@trescenzi you did everything right, do not rename

const url = require('url');
const stripAnsi = require('strip-ansi');
const log = require('loglevel').getLogger('webpack-dev-server');
Expand Down Expand Up @@ -196,7 +196,7 @@ const socketUrl = url.format({
auth: urlParts.auth,
hostname,
port: urlParts.port,
pathname: urlParts.path == null || urlParts.path === '/' ? '/sockjs-node' : urlParts.path
pathname: urlParts.path == null || urlParts.path === '/' ? '/sockjs-node' : (querystring.parse(urlParts.path).sockPath || urlParts.path)
alexander-akait marked this conversation as resolved.
Show resolved Hide resolved
Copy link
Member

Choose a reason for hiding this comment

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

Why we need use querystring.parse here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That I don't know the answer to. It was left over from the initial pr and I kinda just assumed was how stuff was done. Will investigate and come back with an answer.

Copy link
Member

Choose a reason for hiding this comment

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

@trescenzi thanks!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So what's happening here is that __resouceQuery gets the sockPath option passed in. This is then parsing that querystring and getting the sockPath option.

Copy link
Member

Choose a reason for hiding this comment

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

@trescenzi just add this comment to code 👍

});

socket(socketUrl, onSocketMsg);
Expand Down
3 changes: 2 additions & 1 deletion lib/Server.js
Original file line number Diff line number Diff line change
Expand Up @@ -85,6 +85,7 @@ function Server (compiler, options = {}, _log) {

this.watchOptions = options.watchOptions || {};
this.contentBaseWatchers = [];
this.sockPath = `/${options.sockPath ? options.sockPath.replace(/^\/|\/$/g, '') : 'sockjs-node'}`;
Copy link
Member

Choose a reason for hiding this comment

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

Why we need .replace(/^\/|\/$/g, '')?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So this is largely to allow users to provide either: sockPath: '/this/is/a/path/' or sockPath: 'this/is/a/path' and have it behave the same way.

The leading slash gets added by this template string. Instead of checking if there's a leading slash and only adding one if there isn't one, this just always adds a leading slash and removes any if there is one.

The trailing slash is removed because it doesn't mean anything in this case and is likely a result of user error.

Copy link
Member

Choose a reason for hiding this comment

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

@trescenzi please add comment to code with this information in concise form, thanks


// Listening for events
const invalidPlugin = () => {
Expand Down Expand Up @@ -767,7 +768,7 @@ Server.prototype.listen = function (port, hostname, fn) {
});

socket.installHandlers(this.listeningApp, {
prefix: '/sockjs-node'
prefix: this.sockPath
});

if (fn) {
Expand Down
4 changes: 4 additions & 0 deletions lib/options.json
Original file line number Diff line number Diff line change
Expand Up @@ -51,6 +51,9 @@
"socket": {
"type": "string"
},
"sockPath": {
"type": "string"
},
"watchOptions": {
"type": "object"
},
Expand Down Expand Up @@ -321,6 +324,7 @@
"filename": "should be {String|RegExp|Function} (https://webpack.js.org/configuration/dev-server/#devserver-filename-)",
"port": "should be {String|Number} (https://webpack.js.org/configuration/dev-server/#devserver-port)",
"socket": "should be {String} (https://webpack.js.org/configuration/dev-server/#devserver-socket)",
"sockPath": "should be {String} (https://webpack.js.org/configuration/dev-server/#devserver-sockPath)",
Copy link
Member

Choose a reason for hiding this comment

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

This needs to be documented by opening a PR in docs repo please. Currently to URL redirects to the dev server 'landing page', which might be confusing for some and ultimately isn't really helpful

"watchOptions": "should be {Object} (https://webpack.js.org/configuration/dev-server/#devserver-watchoptions)",
"writeToDisk": "should be {Boolean|Function} (https://github.com/webpack/webpack-dev-middleware#writetodisk)",
"headers": "should be {Object} (https://webpack.js.org/configuration/dev-server/#devserver-headers-)",
Expand Down
3 changes: 2 additions & 1 deletion lib/utils/addEntries.js
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,8 @@ function addEntries (config, options, server) {
};

const domain = createDomain(options, app);
const entries = [ `${require.resolve('../../client/')}?${domain}` ];
const sockPath = options.sockPath ? `&sockPath=${options.sockPath}` : '';
const entries = [ `${require.resolve('../../client/')}?${domain}${sockPath}` ];

if (options.hotOnly) {
entries.push(require.resolve('webpack/hot/only-dev-server'));
Expand Down
12 changes: 5 additions & 7 deletions package-lock.json

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

49 changes: 49 additions & 0 deletions test/Socket.test.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,49 @@
'use strict';

const assert = require('assert');
const request = require('supertest');
const config = require('./fixtures/simple-config/webpack.config');
const helper = require('./helper');

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

afterEach((done) => {
helper.close(done);
req = null;
server = null;
});
describe('default behavior', () => {
beforeEach((done) => {
server = helper.start(config, {}, done);
req = request('http://localhost:8080');
});

it('defaults to a path', () => {
assert.ok(server.sockPath.match(/\/[a-z0-9\-/]+[^/]$/));
});

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

describe('socksPath option', () => {
const path = '/foo/test/bar';
beforeEach((done) => {
server = helper.start(config, {
sockPath: '/foo/test/bar/'
}, done);
req = request('http://localhost:8080');
});

it('sets the sock path correctly and strips leading and trailing /s', () => {
assert.equal(server.sockPath, path);
});

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