-
-
Notifications
You must be signed in to change notification settings - Fork 6.3k
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: preserve vite.middlewares connect instance after restarts #15166
feat: preserve vite.middlewares connect instance after restarts #15166
Conversation
server.use((req, res, next) => { | ||
vite.middlewares.handle(req, res, next) | ||
}) | ||
parentServer.use(vite.middlewares) |
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.
This code snippet was wrong, server
should be parentServer
/ecosystem-ci run |
📝 Ran ecosystem CI on
|
const middlewares = server.middlewares | ||
newServer._configServerPort = server._configServerPort | ||
newServer._currentServerPort = server._currentServerPort | ||
Object.assign(server, newServer) | ||
|
||
// Keep the same connect instance so app.use(vite.middlewares) works | ||
// after a restart in middlewareMode (.route is always '/') | ||
middlewares.stack = newServer.middlewares.stack | ||
server.middlewares = middlewares | ||
|
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.
For reference while reviewing the PR, this is the only code change in it. See the first commit: 0a9d8fb
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 don't really have a big counter-argument for this, other than it's making a new property magical. If others are fine with this, I'm also good with it.
We discussed this PR in today's team meeting and decided we should move forward with it. |
Description
Before releasing Vite 5, we decided to update our docs at:
This PR changed the backend integration handling of middlewares from
to
The rationale was that the Vite server instance is already too magical, and we didn't want to make it more magical by preserving the
vite.middlewares
instance.We've been discussing how to update all the projects using the previous recommendation and it is going to be challenging, there may always be bugs. We could rework the old server middelwares connect
stack
to log a warning but in that case, maybe better to just make the previous snippet work properly.This PR rebinds the
.stack
of thenewServer
to preserve the connect instance. In the end, this doesn't look complex to me. And given that at one point we may offer a non-connect-based API, probably better to avoid disruption if the old snippet is easy to fix internally.What is the purpose of this pull request?