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.

Sign up for GitHub

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

feat: support async for proxy.bypass #18940

Merged
merged 7 commits into from
Jan 23, 2025
+76 −21
Merged

feat: support async for proxy.bypass #18940

Changes from 3 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
b8a931b
feat: support async for proxy.bypass
shulaoda Dec 11, 2024
168e5fa
chore: follow the suggestions
shulaoda Dec 11, 2024
0669534
chore: should return when bypass error on event upgrade
shulaoda Dec 11, 2024
5ec1c3d
chore: output error when bypass failed in ws proxy
sapphi-red Dec 12, 2024
be7df7a
chore: update var name
sapphi-red Dec 12, 2024
0d84d54
chore: add test cases
shulaoda Dec 12, 2024
e148ce1
fix: test
shulaoda Dec 12, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
58 changes: 38 additions & 20 deletions packages/vite/src/node/server/middlewares/proxy.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,13 @@ export interface ProxyOptions extends HttpProxy.ServerOptions {
/** undefined for WebSocket upgrade requests */
res: http.ServerResponse | undefined,
options: ProxyOptions,
) => void | null | undefined | false | string
) =>
| void
| null
| undefined
| false
| string
| Promise<void | null | undefined | boolean | string>
/**
* rewrite the Origin header of a WebSocket request to match the target
*
Expand Down Expand Up @@ -158,7 +164,7 @@ export function proxyMiddleware(
})

if (httpServer) {
httpServer.on('upgrade', (req, socket, head) => {
httpServer.on('upgrade', async (req, socket, head) => {
const url = req.url!
for (const context in proxies) {
if (doesProxyContextMatchUrl(context, url)) {
Expand All @@ -169,14 +175,20 @@ export function proxyMiddleware(
opts.target?.toString().startsWith('wss:')
) {
if (opts.bypass) {
const bypassResult = opts.bypass(req, undefined, opts)
if (typeof bypassResult === 'string') {
req.url = bypassResult
debug?.(`bypass: ${req.url} -> ${bypassResult}`)
return
} else if (bypassResult === false) {
debug?.(`bypass: ${req.url} -> 404`)
socket.end('HTTP/1.1 404 Not Found\r\n\r\n', '')
try {
const bypassResult = await opts.bypass(req, undefined, opts)
if (typeof bypassResult === 'string') {
debug?.(`bypass: ${req.url} -> ${bypassResult}`)
req.url = bypassResult
return
}
if (bypassResult === false) {
debug?.(`bypass: ${req.url} -> 404`)
socket.end('HTTP/1.1 404 Not Found\r\n\r\n', '')
return
}
} catch (e) {
sapphi-red marked this conversation as resolved.
Show resolved Hide resolved
debug?.(`bypass: ${req.url} -> ${e}`)
sapphi-red marked this conversation as resolved.
Show resolved Hide resolved
return
}
}
Expand All @@ -194,23 +206,29 @@ export function proxyMiddleware(
}

// Keep the named function. The name is visible in debug logs via `DEBUG=connect:dispatcher ...`
return function viteProxyMiddleware(req, res, next) {
return async function viteProxyMiddleware(req, res, next) {
shulaoda marked this conversation as resolved.
Show resolved Hide resolved
const url = req.url!
for (const context in proxies) {
if (doesProxyContextMatchUrl(context, url)) {
const [proxy, opts] = proxies[context]
const options: HttpProxy.ServerOptions = {}

if (opts.bypass) {
const bypassResult = opts.bypass(req, res, opts)
if (typeof bypassResult === 'string') {
req.url = bypassResult
debug?.(`bypass: ${req.url} -> ${bypassResult}`)
return next()
} else if (bypassResult === false) {
debug?.(`bypass: ${req.url} -> 404`)
res.statusCode = 404
return res.end()
try {
const bypassResult = await opts.bypass(req, res, opts)
if (typeof bypassResult === 'string') {
debug?.(`bypass: ${req.url} -> ${bypassResult}`)
req.url = bypassResult
return next()
}
if (bypassResult === false) {
Copy link

@johncrim johncrim Dec 11, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

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. 

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, but this has to be decided by the maintainer. We need to wait for their review.

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think you can return the same path for that (e.g. bypass: req => req.url).

Agree, this also works

Copy link

Choose a reason for hiding this comment

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

@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?

Copy link

Choose a reason for hiding this comment

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

Here's a webpack dev server test case showing a promise that returns true:

https://github.com/webpack/webpack-dev-server/blob/bcb3725ce40fadae133b78a5b7abbf70eef87031/test/server/proxy-option.test.js#L52

and here's the test referencing it:

https://github.com/webpack/webpack-dev-server/blob/bcb3725ce40fadae133b78a5b7abbf70eef87031/test/server/proxy-option.test.js#L283

Hope that helps.

Copy link

@johncrim johncrim Dec 12, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

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:

https://soofka.pl/entry/mocking-and-proxying-http-requests-from-localhost-with-webpack-dev-server-or-express

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.

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link

@johncrim johncrim Dec 13, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

@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.

Copy link
Member

Choose a reason for hiding this comment

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

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.

debug?.(`bypass: ${req.url} -> 404`)
res.statusCode = 404
return res.end()
}
} catch (e) {
debug?.(`bypass: ${req.url} -> ${e}`)
return next(e)
}
}

Expand Down
Loading