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

resolves #882 #883

Closed
wants to merge 2 commits into from
Closed

Conversation

andrewpmartinez
Copy link

Adds a proxyWsReq event that can be used to modify the outgoing
websocket handshake.

Adds a proxyWsReq event that can be used to modify the outgoing
websocket handshake.
var proxyServer = http.createServer(requestHandler);

proxyServer.on('upgrade', function(req, res, head){
proxy.ws(req, res, head);
Copy link
Contributor

Choose a reason for hiding this comment

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

2-spaces here.

@indexzero
Copy link
Contributor

Thank you for your contribution. I am +1 on this once the minor style things are addressed.

Fixes style issues w/ original http-party#882 additions.
@andrewpmartinez
Copy link
Author

Style issues addressed.

The last change require changing the previous 'var httpProxy' to 'var wsPasses'. Name chosen after seeing the 'var webPasses' for the similar include in the web tests.

@andrewpmartinez
Copy link
Author

If there is anything else you need done before this is accepted; let me know.

@andrewpmartinez
Copy link
Author

When is the next release scheduled?

@indexzero
Copy link
Contributor

@donasaur @jcrugzz thoughts on this?

@donasaur
Copy link
Contributor

donasaur commented Nov 5, 2015

stylistically it looks good. i'm not familiar with this websocket handshake that takes place

the test does show that its able to listen to that proxyres event and add the header tho, and it looks like the destination/source is able to receive that header

@indexzero
Copy link
Contributor

@donasaur it's actually not about the Websocket handshake itself. It is mirroring similar proxyRes functionality which I am fine with. See https://github.com/nodejitsu/node-http-proxy/blob/master/lib/http-proxy/passes/web-incoming.js#L147

@jcrugzz thoughts?

@donasaur
Copy link
Contributor

donasaur commented Nov 5, 2015

ah ok @indexzero , actually it looks like jarrett already merged in a similar PR already:
https://github.com/nodejitsu/node-http-proxy/pull/897/files

can you double check?

@indexzero
Copy link
Contributor

@donasaur excellent. Then this is a duplicate of #897. Closing 💯

@indexzero indexzero closed this Nov 5, 2015
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.

3 participants