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

feat: allow providing parent httpServer on middleware mode #14632

Merged
merged 6 commits into from Nov 15, 2023

Conversation

antfu
Copy link
Member

@antfu antfu commented Oct 15, 2023

Description

resolves #4124

I also encountered a similar issue when developing Nuxt DevTools. As stated in #4124, there is currently no userland workaround as proxyMiddleware isn't exposed publicly. The only way to proxy WebSocket on middleware mode is to re-implement the entire proxy mechanism of Vite, which I think hurts the DX for a built-in feature that is "supposed to work".

Additional context


What is the purpose of this pull request?

  • Bug fix
  • New Feature
  • Documentation update
  • Other

Before submitting the PR, please make sure you do the following

  • Read the Contributing Guidelines.
  • Read the Pull Request Guidelines and follow the PR Title Convention.
  • Check that there isn't already a PR that solves the problem the same way to avoid creating a duplicate.
  • Provide a description in this PR that addresses what the PR is solving, or reference the issue that it solves (e.g. fixes #123).
  • Ideally, include relevant tests that fail without this PR but pass with it.

@antfu antfu added the p3-minor-bug An edge case that only affects very specific usage (priority) label Oct 15, 2023
@bluwy
Copy link
Member

bluwy commented Oct 16, 2023

Pretty sure you've thought about this, but what about overloading as middlewareMode?: boolean | http.Server? The other 'ssr' and 'html' types are deprecated since Vite 3 (#8452) and we can remove them here for Vite 5.

@antfu
Copy link
Member Author

antfu commented Oct 16, 2023

I don't have a strong preference. Was thinking of avoiding bloating the config options, by moving this to an "API specific options" over the general Vite config.

@patak-dev
Copy link
Member

patak-dev commented Oct 16, 2023

I think both options are good. If there is a chance there will later be more options related to middlewareMode, maybe it isn't a bad idea to go for middlewareMode?: boolean | { server: http.Server }.

I am thinking of .server instead of httpServer because we also have server.hmr.server. But ok with being more explicit and add Http there. Should this new middlewareMode parent server be used for HMR too if server.hmr.server isn't provided? If it is only going to be used for the proxy, maybe it could be server.proxyServer (or server.proxy: { server: ..., rules: ... }?)

@bluwy
Copy link
Member

bluwy commented Oct 16, 2023

I think I prefer server.proxy.server too especially that it reflects server.hmr.server. If there's a more general use for http server in middlewareMode, then we can go with middlewareMode?: boolean | { server: http.Server }

@yyx990803
Copy link
Member

I think it's best to avoid an extra option for createServer() - it can be a bit confusing to users regarding why it needs to be in a separate options object.

Overloading middlewareMode as middlewareMode?: boolean | { server: http.Server } makes more sense to me.

@patak-dev patak-dev merged commit e0c86d4 into main Nov 15, 2023
15 checks passed
@patak-dev patak-dev deleted the feat/middleware-mode-http-server branch November 15, 2023 13:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
p3-minor-bug An edge case that only affects very specific usage (priority)
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

WebSocket proxy in middleware mode
4 participants