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

fix(server): respect sockPath on transportMode: 'ws' (#2310) #2311

Merged
merged 2 commits into from Nov 29, 2019

Conversation

joonas-lahtinen
Copy link
Contributor

@joonas-lahtinen joonas-lahtinen commented Nov 9, 2019

  • 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

For Bugs and Features; did you add new tests?

Added tests to verify that sockPath is respected.

Motivation / Use-Case

Fixes bug #2310.

@jsf-clabot
Copy link

jsf-clabot commented Nov 9, 2019

CLA assistant check
All committers have signed the CLA.

@codecov
Copy link

codecov bot commented Nov 9, 2019

Codecov Report

Merging #2311 into master will increase coverage by 0.02%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #2311      +/-   ##
==========================================
+ Coverage   93.84%   93.86%   +0.02%     
==========================================
  Files          34       34              
  Lines        1283     1288       +5     
  Branches      366      367       +1     
==========================================
+ Hits         1204     1209       +5     
  Misses         78       78              
  Partials        1        1
Impacted Files Coverage Δ
lib/servers/WebsocketServer.js 91.66% <100%> (+2.19%) ⬆️

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 3a31917...976e0cf. Read the comment docs.

Copy link
Member

@alexander-akait alexander-akait left a comment

/cc @Loonride

@alexander-akait
Copy link
Member

alexander-akait commented Nov 9, 2019

Please accept CLA

this.wsServer.on('error', (err) => {
this.server.log.error(err.message);
});
}

shouldUpgrade(req) {
const pathname = url.parse(req.url).pathname;
Copy link
Member

@hiroppy hiroppy Nov 9, 2019

Choose a reason for hiding this comment

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

memo: we'll use URLSearchParams after major release.

Copy link
Contributor Author

@joonas-lahtinen joonas-lahtinen Nov 9, 2019

Choose a reason for hiding this comment

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

I fixed the code to use WebSocket.Server.shouldHandle()
https://github.com/websockets/ws/blob/master/doc/ws.md#servershouldhandlerequest
Should be more futureproof?

@joonas-lahtinen joonas-lahtinen force-pushed the fix/transportMode-ws-proxy branch from ee3e805 to 6bcddc9 Compare Nov 9, 2019
@joonas-lahtinen
Copy link
Contributor Author

joonas-lahtinen commented Nov 9, 2019

Thanks for the quick reviews. I noticed a bug in the tests based on the code coverage difference report, should be fixed now and using the more forward compatible ws.Server.shouldHandle()

@joonas-lahtinen
Copy link
Contributor Author

joonas-lahtinen commented Nov 9, 2019

This also fixes the bug that whole webpack-dev-server process exits due to an unhandled error if the proxy target is not open:

[HPM] Rewriting path from "/api" to ""
[HPM] GET /api ~> http://localhost:8888
[HPM] Upgrading to WebSocket
events.js:174
      throw er; // Unhandled 'error' event
      ^

Error [ERR_STREAM_DESTROYED]: Cannot call write after a stream was destroyed

alexander-akait
alexander-akait previously approved these changes Nov 11, 2019
Copy link
Member

@alexander-akait alexander-akait left a comment

/cc @hiroppy @Loonride wait both reviews

Copy link
Collaborator

@knagaitsev knagaitsev left a comment

@joonas-lahtinen I ran your new tests without the changes that you made, and they passed. How are your changes different from the existing behavior? The ws docs say for the path option - "Accept only connections matching this path" (https://github.com/websockets/ws/blob/master/doc/ws.md#new-websocketserveroptions-callback), so paths that do not match the one specified are already rejected.

Edit: I missed the explanation in #2310. Could you make a test demonstrating that proxying works?

@joonas-lahtinen
Copy link
Contributor Author

joonas-lahtinen commented Nov 13, 2019

Thanks for the patience. After some digging I found the spot to inject testing the different transportMode options with proxy.

@knagaitsev
Copy link
Collaborator

knagaitsev commented Nov 20, 2019

LGTM /cc @evilebottnawi @hiroppy

Copy link
Member

@alexander-akait alexander-akait left a comment

/cc @hiroppy

@hiroppy hiroppy merged commit e188542 into webpack:master Nov 29, 2019
17 checks passed
@hiroppy
Copy link
Member

hiroppy commented Nov 29, 2019

thanks

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants