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

Merged
merged 19 commits into from
Jan 30, 2019
Merged

feat: add sockPath option (options.sockPath) #1553

Show file tree
Hide file tree
Changes from 7 commits
Commits
Show all changes
19 commits
Select commit Hold shift + click to select a range
01054e8
explicit sockPath
cheapsteak Jan 22, 2018
27fb6a5
add sockPath to options
cheapsteak Jan 22, 2018
4bbc1f6
fix: remove leading and trailing slashes for sockPath
cheapsteak Jan 22, 2018
e6ab166
add sockPath to auto-added entry points in /bin
cheapsteak Jan 22, 2018
d52103b
add examples referencing kellyrmilligan’s pull/911
cheapsteak Jan 22, 2018
4a07096
fix test
cheapsteak Jan 22, 2018
4078156
fix lint errors + correct messed up rebase
trescenzi Oct 29, 2018
745908f
fix sockjs path formatting
trescenzi Oct 31, 2018
c0faeeb
remove example
trescenzi Oct 31, 2018
bd31f5a
add sockpath tests
trescenzi Oct 31, 2018
e1d41e0
fix tests
trescenzi Oct 31, 2018
b2ef4de
add real tests for sockPath
trescenzi Oct 31, 2018
1327568
fix lint errors
trescenzi Oct 31, 2018
bf67dd6
Merge branch 'master' into fix/socksjs-path-2
evilebottnawi Dec 5, 2018
8332f4f
Merge branch 'master' into fix/socksjs-path-2
trescenzi Dec 5, 2018
a3d320e
refactor socket tests to use super test
trescenzi Dec 5, 2018
f384993
Merge branch 'master' into fix/socksjs-path-2
trescenzi Dec 7, 2018
7542c35
Merge branch 'master' into fix/socksjs-path-2
evilebottnawi Jan 30, 2019
d252f19
add comments for clarification about sockPath code
trescenzi Jan 30, 2019
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
View file
Open in desktop
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 •
edited

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
15 changes: 15 additions & 0 deletions examples/node-api-sock-path/README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,15 @@
# Node.js API - Simple
Copy link
Member

Choose a reason for hiding this comment

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

Please remove all examples as we don't intend to maintain them in the future (#1469)


```shell
node server.js
```

Starts a simple webpack-dev-server setup via the Node API. Open `http://localhost:8080/` to go the app.

## What should happen

In the app you should see "It's working."

In `app.js`, uncomment the code that results in an error and save. This error should be visible in the CLI and devtools.

Then, in `app.js`, uncomment the code that results in a warning. This warning should be visible in the CLI and devtools.
3 changes: 3 additions & 0 deletions examples/node-api-sock-path/app.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
'use strict';

document.write('It\'s working under a subapp');
9 changes: 9 additions & 0 deletions examples/node-api-sock-path/index.html
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
<!DOCTYPE html>
<html>
<head>
<script src="/subapp/bundle.js" type="text/javascript" charset="utf-8"></script>
</head>
<body>
<h1>Example: Node.js API - Simple</h1>
</body>
</html>
25 changes: 25 additions & 0 deletions examples/node-api-sock-path/server.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,25 @@
'use strict';

const path = require('path');
const Webpack = require('webpack');
const WebpackDevServer = require('../../lib/Server');
const webpackConfig = require('./webpack.config');

const compiler = Webpack(webpackConfig);
const server = new WebpackDevServer(compiler, {
stats: {
colors: true
},
contentBase: path.resolve(__dirname),
watchContentBase: true,
sockPath: '/subapp/sockjs-node',
publicPath: '/subapp/',
historyApiFallback: {
disableDotRule: true,
index: '/subapp/'
}
});

server.listen(8080, '127.0.0.1', () => {
console.log('Starting server on http://localhost:8080');
});
10 changes: 10 additions & 0 deletions examples/node-api-sock-path/webpack.config.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
'use strict';

module.exports = {
context: __dirname,
entry: ['./app.js', '../../client/index.js?http://localhost:8080/&sockPath=subapp/sockjs-node'],
output: {
filename: 'bundle.js',
publicPath: '/subapp/'
}
};
3 changes: 2 additions & 1 deletion lib/Server.js
View file
Open in desktop
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(/^\/|\/$/, '') : 'sockjs-node'}`;

// Listening for events
const invalidPlugin = () => {
Expand Down Expand Up @@ -763,7 +764,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
View file
Open in desktop
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
View file
Open in desktop
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