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

Maintenance #1251

Merged
merged 25 commits into from Apr 20, 2018
Merged

Maintenance #1251

merged 25 commits into from Apr 20, 2018

Conversation

jcrugzz
Copy link
Contributor

@jcrugzz jcrugzz commented Apr 19, 2018

Running branch for merging PRs into that will properly run tests and pins dependencies as it should be.

@jcrugzz jcrugzz requested review from donasaur and indexzero Apr 20, 2018
Copy link
Contributor

@donasaur donasaur left a comment

My review was pretty shallow since my memory of this codebase is pretty rough.

@jcrugzz Good luck!

README.md Outdated
* `false` (default): disable cookie rewriting
* String: new path, for example `cookiePathRewrite: "/newPath/"`. To remove the path, use `cookiePathRewrite: ""`. To set path to root use `cookiePathRewrite: "/"`.
* Object: mapping of paths to new paths, use `"*"` to match all paths.
For example keep one path unchanged, rewrite one path and remove other paths:
Copy link
Contributor

@donasaur donasaur Apr 20, 2018

Choose a reason for hiding this comment

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

Minor nit: keep -> to keep

README.md Outdated
* **proxyTimeout**: timeout (in millis) for outgoing proxy requests
* **timeout**: timeout (in millis) for incoming requests
* **followRedirects**: true/false, Default: false - specify whether you want to follow redirects
* **selfHandleRequest** true/false, if set to true, none of the webOutgoing passes are called and its your responsibility ro appropriately return the response by listening and acting on the `proxyRes` event
Copy link
Contributor

@donasaur donasaur Apr 20, 2018

Choose a reason for hiding this comment

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

it's

README.md Outdated
@@ -442,6 +486,33 @@ proxy.close();

### Miscellaneous

If you want to handle your own response after receiving the proxyRes, you can do
so with `selfHandleResponse`
Copy link
Contributor

@donasaur donasaur Apr 20, 2018

Choose a reason for hiding this comment

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

A bit more clarification would be helpful here:
Not sure what it means to handle my own response. Does this mean that I will return my own response to the proxy client? Does the response have to be a modified version of the target webpage response?

A more descriptive var name would also be helpful (e.g., returnCustomResponse).

Copy link
Contributor

@donasaur donasaur Apr 20, 2018

Choose a reason for hiding this comment

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

or is selfHandleRequest is the options above the same as selfHandleResponse?

Copy link
Contributor Author

@jcrugzz jcrugzz Apr 20, 2018

Choose a reason for hiding this comment

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

@donasaur yea good catch it is supposed to just be selfHandleResponse in general. The feature is basically you are taking responsibility for returning whatever you want to the original response after the proxyRequest happens and returns its response. This was the name given by the contributor I'm ok with finding something that makes more sense. Its just trying to indicate that its your responsibility to return the response if thats something you needed to do. modifyResponse is also another idea.

Copy link
Contributor

@Tigge Tigge left a comment

Can remove the Change-Id: I142a15ee12603f54d611ae9362a94c62fb3c9f36 from my commit. It's a Gerrit change id which accidentally made its way into there.

Tigge and others added 6 commits Apr 20, 2018
When the server do not accept the upgrade request for websockets the
server's response was previously not included and sent back. Now the
proxy will include the response in these cases. Fixes #890.
This PR tries to fix "Can't set headers after they are sent" errors.
That are a lot of situations where this error can occurs. In my case, it is happening because I have others middlewares (in an expressjs application that tries to proxy requests). Some of those middlewares (like [passportjs](http://passportjs.org/), or [cors](https://www.npmjs.com/package/cors)) can run ```res.end()``` and when the proxy receive a response, it is already finished.
So, it is necessary to test if we can write on the user response when the proxy response is ready.
I think it could also fix #930, #1168, #908
object.keys in web-incoming.js results in a non-deterministic ordering of keys, which means that in web-outgoing writeHead might be called before setHeader, which throws an error
…lfHandleRequest is the only way that functionality works
@jcrugzz
Copy link
Contributor Author

@jcrugzz jcrugzz commented Apr 20, 2018

@Tigge removed that line from the commit. Again, thanks for the contribution!

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