-
-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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
Changes from all commits
01054e8
27fb6a5
4bbc1f6
e6ab166
d52103b
4a07096
4078156
745908f
c0faeeb
bd31f5a
e1d41e0
b2ef4de
1327568
bf67dd6
8332f4f
a3d320e
f384993
7542c35
d252f19
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -2,7 +2,7 @@ | |
|
||
/* global __resourceQuery WorkerGlobalScope self */ | ||
/* eslint prefer-destructuring: off */ | ||
|
||
const querystring = require('querystring'); | ||
const url = require('url'); | ||
const stripAnsi = require('strip-ansi'); | ||
const log = require('loglevel').getLogger('webpack-dev-server'); | ||
|
@@ -196,7 +196,10 @@ const socketUrl = url.format({ | |
auth: urlParts.auth, | ||
hostname, | ||
port: urlParts.port, | ||
pathname: urlParts.path == null || urlParts.path === '/' ? '/sockjs-node' : urlParts.path | ||
// If sockPath is provided it'll be passed in via the __resourceQuery as a | ||
// query param so it has to be parsed out of the querystring in order for the | ||
// client to open the socket to the correct location. | ||
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
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why we need use There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @trescenzi thanks! There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. So what's happening here is that There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @trescenzi just add this comment to code 👍 |
||
}); | ||
|
||
socket(socketUrl, onSocketMsg); | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -101,6 +101,8 @@ function Server (compiler, options = {}, _log) { | |
|
||
this.watchOptions = options.watchOptions || {}; | ||
this.contentBaseWatchers = []; | ||
// Replace leading and trailing slashes to normalize path | ||
this.sockPath = `/${options.sockPath ? options.sockPath.replace(/^\/|\/$/g, '') : 'sockjs-node'}`; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why we need There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. So this is largely to allow users to provide either: 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 = () => { | ||
|
@@ -798,7 +800,7 @@ Server.prototype.listen = function (port, hostname, fn) { | |
}); | ||
|
||
socket.installHandlers(this.listeningApp, { | ||
prefix: '/sockjs-node' | ||
prefix: this.sockPath | ||
}); | ||
|
||
if (fn) { | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -51,6 +51,9 @@ | |
"socket": { | ||
"type": "string" | ||
}, | ||
"sockPath": { | ||
"type": "string" | ||
}, | ||
"watchOptions": { | ||
"type": "object" | ||
}, | ||
|
@@ -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)", | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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-)", | ||
|
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
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); | ||
}); | ||
}); | ||
}); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.There was a problem hiding this comment.
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