feat: support async for proxy.bypass #18940
Conversation
|
Thank you @shulaoda! I appreciate your quick response on this. |
| req.url = bypassResult | ||
| return next() | ||
| } | ||
| if (bypassResult === false) { |
There was a problem hiding this comment.
There needs to be a supported way for the bypass fn to handle the request fully, without resulting in a 404 or a different path. Webpack supports that, and it's in the name "bypass".
I liked your earlier proposal to check if the response has already been written before eg setting the status code.
There was a problem hiding this comment.
Yes, but this has to be decided by the maintainer. We need to wait for their review.
There was a problem hiding this comment.
There needs to be a supported way for the bypass fn to handle the request fully, without resulting in a 404 or a different path.
I think you can return the same path for that (e.g. bypass: req => req.url).
Changing the behavior for bypassResult === false to res.statusCode = 404; req.url = ""; next(); breaks the current behavior and makes the behavior different from the webpack's docs, so I think we shouldn't do that.
https://github.com/webpack/webpack-dev-server/blob/master/DOCUMENTATION-v4.md#devserverproxy:~:text=Return%20false%20to%20produce%20a%20404%20error%20for%20the%20request.
Webpack supports that
Webpack's docs does not describe anything when bypassResult was other than null / undefined / false / string, so I think you just relied on a undefined behavior and happened to work in your case.
https://github.com/webpack/webpack-dev-server/blob/master/DOCUMENTATION-v4.md#devserverproxy:~:text=In%20the%20function,proxy%20the%20request
There was a problem hiding this comment.
I think you can return the same path for that (e.g.
bypass: req => req.url).
Agree, this also works
There was a problem hiding this comment.
@sapphi-red - I see that it's not in the docs that you reference, but I bet those couple of lines aren't the only docs. Are you saying there should be no way to handle the request in the bypass function? Doesn't the name "bypass" imply that the function can bypass the proxy?
If that's not part of the reason for its existence, then how, in the proxy config, do you short-circuit the request and return a response?
There was a problem hiding this comment.
Here's a webpack dev server test case showing a promise that returns true:
and here's the test referencing it:
Hope that helps.
There was a problem hiding this comment.
There are quite a few other examples of using webpack dev server bypass to send the response. The example on this page doesn't return true, it just handles the response:
The test cases handle the cases I care about. The only part I'm disputing now is whether returning the request path, after handling the request, makes sense.
There was a problem hiding this comment.
Are you saying there should be no way to handle the request in the bypass function?
I won't say there should be no way to do that in the bypass function, because the bypass function can already do that by bypass: req => req.url. But IMO it's not necessary to have that functionality in the bypass function. Vite allows users to configure the middlewares, and handling a request before the proxy middleware can be done by that. The bypass function should focus on bypassing the proxy middleware and make those handled by the other internal middlewares of Vite.
I don't know how much of a goal it is to maintain compatibility with webpack, since it's a moving target (now webpack v5 is pushing users to the http-proxy-middleware api).
The proxy options that Vite additionally has over the underlying http-proxy library are not documented well and kind a like "the implementation defines the spec" thing. It does have any description about how each options are meant to be used and difficult to assume how users will be using the options. I'm trying to take a balance of unblocking you but still not breaking others here. It'd be great if we can overhaul the options and clarify how each options is meant to be used, but that would take adequate time.
There was a problem hiding this comment.
@sapphi-red - thank you for the response. My company can only use the proxy API to handle responses - we can't use vite plugins/middleware, b/c Angular exposes the vite proxy API, but doesn't allow vite plugins to be added.
I'm trying to take a balance of unblocking you but still not breaking others here. It'd be great if we can overhaul the options and clarify how each options is meant to be used, but that would take adequate time.
Understood, I really appreciate the explanation.
There was a problem hiding this comment.
we can't use vite plugins/middleware, b/c Angular exposes the vite proxy API, but doesn't allow vite plugins to be added.
I think Angular should expose an additional feature on their side; it doesn't feel right to me to create duplicate surfaces on the Vite side for a workaround just because Angular limited the surfaces.
sapphi-red
added
the
p2-nice-to-have
|
Please wait, I will add the relevant test cases. |
shulaoda
force-pushed
the
feat/support-async-for-proxy-bypass
branch
from
b879bc0 to
0d84d54
Compare
| 'Content-Type': 'text/plain', | ||
| }) | ||
| res.end('Hello after 4 ms (async timeout)') | ||
| return '/asyncResponse' |
There was a problem hiding this comment.
I think it's odd to have to return the path of the proxy as the return value here, after the request for that path has been handled. I think user's would expect either returning true or void (no return) should work.
But otherwise this completely handles the cases I care about - thank you!
There was a problem hiding this comment.
@sapphi-red - please let us know whether you still think returning the path that was just handled makes sense.
Description
closes #18920
I will add relevant tests after resolving all comments.