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) #1289

Closed
wants to merge 7 commits into from

Conversation

cheapsteak
Copy link

@cheapsteak cheapsteak commented Jan 22, 2018

  • This is a bugfix
  • This is a code refactor
  • This is a test update
  • This is a typo fix
  • This is a metadata update

For Bugs and Features; did you add new tests?

Added new tests for addDevServerEntrypoints

I've added examples referenced from #911

I've tested this against the breaking example provided in 2.7.0 and it doesn't suffer the same failure.

Motivation / Use-Case

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

Not sure if v3 fixes this and not sure how long until v3 comes out. This should fix our use case and shouldn't break anything else in the mean time

Breaking Changes

None

Additional Info

It doesn't rely on the publicPath value, but unfortunately the sockPath value has to be passed twice, once in the devServer option (this is what Server.js uses to determine what prefix to listen at), and once again in the entry resource url as a query parameter (this is what client/index.js uses to determine where to send requests to)

@jsf-clabot
Copy link

jsf-clabot commented Jan 22, 2018

CLA assistant check
All committers have signed the CLA.

@cheapsteak cheapsteak changed the title Fix/sockjs path 2 Add sockPath option to allow specifying alternatives to /sockjs-node Jan 22, 2018
@codecov
Copy link

codecov bot commented Jan 22, 2018

Codecov Report

Merging #1289 into master will increase coverage by 2.78%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1289      +/-   ##
==========================================
+ Coverage   75.72%   78.51%   +2.78%     
==========================================
  Files           5        5              
  Lines         482      484       +2     
  Branches      156      158       +2     
==========================================
+ Hits          365      380      +15     
+ Misses        117      104      -13
Impacted Files Coverage Δ
lib/Server.js 82.02% <100%> (+0.05%) ⬆️
lib/util/addDevServerEntrypoints.js 94.11% <100%> (+81.61%) ⬆️

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 83c1625...a4f35e3. Read the comment docs.

@burtek
Copy link

burtek commented Feb 16, 2018

@cheapsteak Does this allow to explicitely set hostname or at least port for socket? I need the socket to connect to a dev server on localhost:3000 from inside another dev site hosted on localhost:80

@cheapsteak
Copy link
Author

@burtek unfortunately that is not part of this PR

I believe you should be able to reference my changes and add additional fields for what you'd need (e.g. sockPort and sockHost)

@burtek
Copy link

burtek commented Feb 16, 2018

@cheapsteak got it, thanks for answer

@Jokero
Copy link

Jokero commented Apr 20, 2018

Also found that I need a way to change the port for sockjs)

@Jokero
Copy link

Jokero commented Apr 20, 2018

Ah, you don't use inline mode. Otherwise there is already public option https://webpack.js.org/configuration/dev-server/#devserver-public

@cheapsteak
Copy link
Author

The public option only affects where the client tries to connect to, but the server is hard coded to listen at '/sockjs-node'

@iphands
Copy link

iphands commented May 30, 2018

The Motivation / Use-Case is spot on here! Thanks @cheapsteak

I believe you should be able to reference my changes and add additional fields for what you'd need (e.g. sockPort and sockHost)

That part could be a separate PR, but it is also very much needed! The reverse proxy can use a completely different hostname and port. For me the hostname override is not needed... I think setting the w-d-s host to makes the behavior change such that the override is not necessary, that might be accidental behavior though.

@flyyang
Copy link

flyyang commented May 31, 2018

What's the status of this PR?

iphands added a commit to RedHatInsights/frontend-starter-app that referenced this pull request May 31, 2018
…with WDS

Webpack Dev Server had some bad issues with their websocket for hot reloading setup.
They made some assumptions about what the servers base URL, HOST, and PORT and this was *not* configurable by the user.
This made the hot reloading features fail hard when used through a proxy like insights-proxy
See: webpack/webpack-dev-server#1289

webpack-serve does not have these same issues
iphands added a commit to RedHatInsights/frontend-starter-app that referenced this pull request May 31, 2018
…with WDS

Webpack Dev Server had some bad issues with their websocket for hot reloading setup.
They made some assumptions about what the servers base URL, HOST, and PORT and this was *not* configurable by the user.
This made the hot reloading features fail hard when used through a proxy like insights-proxy
See: webpack/webpack-dev-server#1289

webpack-serve does not have these same issues
@iphands
Copy link

iphands commented May 31, 2018

@flyyang I converted the project I am working on to use webpack-serve. That project seemed to do the "right thing" regarding proxied web sockets (for hot reloading) straight out of the box.

iphands added a commit to RedHatInsights/frontend-starter-app that referenced this pull request Jun 1, 2018
…with WDS

Webpack Dev Server had some bad issues with their websocket for hot reloading setup.
They made some assumptions about what the servers base URL, HOST, and PORT and this was *not* configurable by the user.
This made the hot reloading features fail hard when used through a proxy like insights-proxy
See: webpack/webpack-dev-server#1289

webpack-serve does not have these same issues
@michael-ciniawsky michael-ciniawsky changed the title Add sockPath option to allow specifying alternatives to /sockjs-node feat: add sockPath option (options.sockPath) Aug 21, 2018
@michael-ciniawsky michael-ciniawsky added this to the 3.2.0 milestone Aug 21, 2018
@codecov
Copy link

codecov bot commented Sep 18, 2018

Codecov Report

Merging #1289 into master will increase coverage by 2.78%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1289      +/-   ##
==========================================
+ Coverage   75.72%   78.51%   +2.78%     
==========================================
  Files           5        5              
  Lines         482      484       +2     
  Branches      156      158       +2     
==========================================
+ Hits          365      380      +15     
+ Misses        117      104      -13
Impacted Files Coverage Δ
lib/Server.js 82.02% <100%> (+0.05%) ⬆️
lib/util/addDevServerEntrypoints.js 94.11% <100%> (+81.61%) ⬆️

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 83c1625...a4f35e3. Read the comment docs.

@trescenzi
Copy link
Contributor

trescenzi commented Oct 26, 2018

Hi what's the status of this PR? Seems like it fell out because webpack-serve was doing the right thing. But now that webpack-serve is deprecated in favor of webpack-dev-server. Is there any chance this could get merged?

I'd be happy to help get it up to date with master and make the changes, host/port, mentioned in this comment.

@THernandez03
Copy link

@cheapsteak Does this allow to explicitely set hostname or at least port for socket? I need the socket to connect to a dev server on localhost:3000 from inside another dev site hosted on localhost:80

That should be very nice too, be able to determine the entire url of the socket, not only the path.

@alexander-akait
Copy link
Member

Need rebase or create new Pr

@trescenzi
Copy link
Contributor

@evilebottnawi I've got it up to date in my fork. Don't want to just commandeer this though. Especially without confirmation it'll be accepted.

@cheapsteak
Copy link
Author

Commandeer away! I'll close this PR

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.

10 participants