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

@michael-ciniawsky michael-ciniawsky Oct 30, 2018

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
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
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
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
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
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
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