-
Notifications
You must be signed in to change notification settings - Fork 63
Support overriding upstream HTTP method #134
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
base: master
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I, personally, have no use for it, but why not.
@@ -92,7 +92,7 @@ internals.handler = function (route, handlerOptions) { | |||
|
|||
return async function (request, h) { | |||
|
|||
const { uri, headers } = await settings.mapUri(request); | |||
let { uri, headers, method } = await settings.mapUri(request); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd rather keep it that way:
let { uri, headers, method } = await settings.mapUri(request); | |
const { uri, headers, method = request.method } = await settings.mapUri(request); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The proposed change by @Marsup would break my existing code, that changes request.method
in mapUri()
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wait, will it break? I assumed the it fetches the request.method
before calling mapUri()
, but that is probably not the case, since it needs the result to know whether to fetch the default.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd say it's working as expected: https://runkit.com/embed/03jrtcm2e10x
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great! Reminder to self: Get my morning cup of coffee, before reviewing code.
@@ -47,6 +47,7 @@ The proxy handler object has the following properties: | |||
* `request` - is the incoming [request object](http://hapijs.com/api#request-object). The response from this function should be an object with the following properties: | |||
* `uri` - the absolute proxy URI. | |||
* `headers` - optional object where each key is an HTTP request header and the value is the header content. | |||
* `method` - optional HTTP method to use when making request to upstream server. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we add a word that it defaults to the same verb as the route?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There are definitely use-cases for changing the HTTP verb. I have some code, which already does this in a hacky way, by changing the value of request.method
in a mapUri()
hook.
However, this PR won't help me much, since I'm changing a web form GET
to the equivalent POST
. This transform requires the query parameters to be serialized into the request.payload
, which I still can only do by modifying the request.payload
during mapUri()
.
Maybe you can support a payload
return value as well?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM and I agree with the feedback.
I was thinking this can be a useful feature for when you want to replace a legacy API that might only support POST with more RESTful methods via the proxy. I don't know if others will find this valuable or not, so I can go either way on if we want to add it.