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

Conversation

trescenzi
Copy link
Contributor

  • This is a bugfix
  • This is a feature
  • This is a code refactor
  • This is a test update
  • This is a docs update
  • This is a metadata update

This is a version of #1289 that's up to date with master.

For Bugs and Features; did you add new tests?

The tests that were added in #1289 seem to exist in master now. I can add more specific tests if desired.

Motivation / Use-Case

copied from #1289; although I have the same motivation
Similar motivations as #911.

We also have multiple apps being developed behind a reverse proxy (in our case a docker composed nginx). Each app is served from a top-level sub-directory (can't use root path), e.g. host/app-part-one, host/app-another-piece

Breaking Changes

None

Additional Info

There was discussion on #1289 about adding port and host to the socks options. I'm happy to do so if there's still interest. I didn't include it here though because this happens to be all I need.

@jsf-clabot
Copy link

jsf-clabot commented Oct 29, 2018

CLA assistant check
All committers have signed the CLA.

Copy link
Member

@alexander-akait alexander-akait left a comment

Choose a reason for hiding this comment

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

Looks good, need add tests

@michael-ciniawsky michael-ciniawsky changed the title Feature: Add SocksPath to config feat: add sockPath option (options.sockPath) Oct 30, 2018
Copy link
Member

@michael-ciniawsky michael-ciniawsky left a comment

Choose a reason for hiding this comment

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

@trescenzi Thx in advance

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

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

client-src/default/index.js Show resolved Hide resolved
@@ -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

@codecov
Copy link

codecov bot commented Oct 31, 2018

Codecov Report

Merging #1553 into master will increase coverage by 0.07%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1553      +/-   ##
==========================================
+ Coverage   74.41%   74.49%   +0.07%     
==========================================
  Files          10       10              
  Lines         688      690       +2     
==========================================
+ Hits          512      514       +2     
  Misses        176      176
Impacted Files Coverage Δ
lib/Server.js 81.53% <100%> (+0.04%) ⬆️
lib/utils/addEntries.js 100% <100%> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 11c9896...d252f19. Read the comment docs.

@trescenzi
Copy link
Contributor Author

I've:

  • removed the example
  • added tests
  • fixed a bug noted while testing

Still need to update the webpack documentation itself with another PR.

Let me know if there's more to do here.

Also do you want these commits squashed or are they ok as is?

Copy link
Member

@alexander-akait alexander-akait left a comment

Choose a reason for hiding this comment

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

Looks good, let's wait CI green, after merge need create PR in webpack docs repo

@trescenzi
Copy link
Contributor Author

Huh so I was running the tests with a .only looks like they fail when run with all tests. I'll investigate that.

@trescenzi
Copy link
Contributor Author

The test issues are only in node 11 it seems. I'm still investigating but there isn't anything obvious in the node 11 release notes that would indicate that child process would behave differently.

@alexander-akait
Copy link
Member

@trescenzi can you reproducible test failed locally?

@trescenzi
Copy link
Contributor Author

@evilebottnawi I can reproduce the hanging CLI tests locally. I'm busy over the next few days but I can take a look again on monday.

@alexander-akait
Copy link
Member

@trescenzi can you provide details how you can reproduce problem?

@trescenzi
Copy link
Contributor Author

@evilebottnawi all I have to do, on master, is:

> brew switch node 11.0.0
> rm -rf node_modules
> npm install
> npm test

Sometimes it hangs during the tests, sometimes it segfaults. I don't have issues with node 10.12.0.

@alexander-akait
Copy link
Member

@trescenzi thanks!

@trescenzi
Copy link
Contributor Author

My apologies I've been away from this for so long. I've got time over the next few days to figure out what's up and get this out.

@odinho
Copy link
Contributor

odinho commented Dec 4, 2018

@trescenzi If you rebase to newest master, Node 11 is disabled for now. I did see one hang on Node 10 too, but I don't run Mac OS X, so it was hard to debug.

I've opened a bug on node.js, if you potentially want to try this with llnode to get a full stack trace as commented in the bug: nodejs/node#24835

@trescenzi
Copy link
Contributor Author

trescenzi commented Dec 5, 2018

When I was trying to get the tests working before I made so updates to the tests for this change that used supertest more idiomatically instead of relying upon request. I've pushed these changes. Should just have to click the update branch button and this will be good to go. Waiting to click though in case it gets out of date again between now and when it's good to merge.

@odinho definitely interested in debugging node. It's been too long since I've attached gdb to a running process haha. Not sure if I have time this moment to do so though. I'll see when I end up today. The issues are still 100% reproducible in the tests just by switching to node 11.

@trescenzi
Copy link
Contributor Author

Any updates here? As far as I'm aware it should be good to merge. Let me know if there's anything else I can do.

Copy link
Member

@alexander-akait alexander-akait left a comment

Choose a reason for hiding this comment

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

Just couple questions and we can merge

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

@@ -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)
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 👍

@zsalzbank
Copy link

zsalzbank commented Jan 30, 2019

While waiting for this, I came up with a temporary workaround (assuming you are using webpack). You'll need the file in this gist: https://gist.github.com/zsalzbank/cdee4f852e4a89c116eb7a28491bbf63

Then, add the exported plugin to your babel plugins like so:

...
plugins: [
  require('sockjs-prefix-modification')('/my-prefix-path/').plugin
],
...

And before you import webpack-dev-server for your server, run the following:

require('sockjs-prefix-modification')('/my-prefix-path/').fixWebpackDevServer();

You'll need @babel/register and babel-plugin-search-and-replace in your devDependencies.

@alexander-akait
Copy link
Member

/cc @trescenzi can you fix note from me above and we can merge

@trescenzi
Copy link
Contributor Author

My apologies for taking forever to update this. It dropped off my radar. It should be good now.

I'm going to put up a PR against the docs site now.

@trescenzi
Copy link
Contributor Author

Looks like the linting hung? It lints fine for me locally.

@alexander-akait
Copy link
Member

alexander-akait commented Jan 30, 2019

@trescenzi yep, something wrong with travis ci, rerun CI

trescenzi added a commit to trescenzi/webpack.js.org that referenced this pull request Jan 30, 2019
@alexander-akait alexander-akait merged commit 4bf1f76 into webpack:master Jan 30, 2019
@trescenzi trescenzi deleted the fix/socksjs-path-2 branch January 30, 2019 18:51
carlosgeos pushed a commit to carlosgeos/webpack-dev-server that referenced this pull request Feb 26, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants