-
-
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: migrate from connect to polka #17569
Conversation
Run & review this pull request in StackBlitz Codeflow. |
Migrates from connect to polka, a much lighter and faster middleware server/router. Marked as WIP until we figure out what to do about fallback middleware.
Switches to polka 1.x which orders middleware the same way as express, allowing us to have fallback middleware (0.x doesn't allow multiple fallbacks).
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.
Question: With Vite's current connect-style middleware interface, is it not possible for the consumer to use Polka with it? Are there other blockers than the error middleware difference?
(I noticed you edited away some previous concerns you had. Do they still apply now?)
One concern with transforming all the internal middlewares to Polka style is that Polka actually extends more of the req
than connect:
// polka
export interface Request extends IncomingMessage {
url: string
method: string
originalUrl: string
params: Record<string, string>
path: string
search: string
query: Record<string, string>
body?: any
_decoded?: true
_parsedUrl: ParsedURL
}
// connect
export class IncomingMessage extends http.IncomingMessage {
originalUrl?: http.IncomingMessage['url'] | undefined
}
Middlewares should work at the lowest common denominator, especially that Vite plugins can add them, and consumers can hook them to any connect-compatible servers.
middlewares.stack = newServer.middlewares.stack | ||
middlewares.wares = newServer.middlewares.wares |
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 is likely a big breaking change. There's a fair share of metaframeworks that uses .stack
to manually turn off some of Vite's 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.
not sure you can really avoid that without patching our instance of polka to have a "fake" stack
(which is just an alias of wares
)
also sounds like vite should be exposing the stack of middleware itself in future. this feels like an internal property people have ended up depending on, when really vite itself should probably have a top-level server.stack
or server.middlewareStack
etc
before, it locked us into express (because stack
belongs to express' interface, not ours). now it locks us into polka. we shouldn't be locked into anything, there should be a higher level exposed stack abstracted from the server we choose to use imo
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.
Vite isn't depending on express API, it's from connect and we document exposing a connect instance via viteServer.middlewares
. The main goal from that is that any connect-compatible servers, like express, can easily do something like server.use(viteServer.middlewares)
. Whatever else exported from viteServer.middlewares
isn't locking us to a specific server-framework IMO, except connect.
middlewares.use(errorMiddleware(server, !!middlewareMode)) | ||
middlewares.onError = errorMiddleware(server, !!middlewareMode) |
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 is also a big breaking change I think. It will limit users to only able to use Polka? If they prefer to use Express, could they still do so?
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.
do you have an example?
you mean they'd create a vite instance and overwrite the middleware with express?
think i need to understand where you think we're making this polka-only, and where users would be passing express in
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.
Let's say if we use the previous express setups in the playground/
and keep the polka changes from core, do they still work together? My fear is that if the have:
const app = express()
app.use(vite.middlewares)
The error handling would no longer work. Or maybe I'm mistaken.
moving to polka 1.x solved all of these (1.x orders middleware the same way express/connect do)
connect/express cause the same problem but with we could just dumb the type down if we don't want to publicly support the extensions |
Good to know that polka v1 solves them 👍
According to the previous connect types, I don't think they do? export type SimpleHandleFunction = (
req: IncomingMessage,
res: http.ServerResponse,
) => void
export type NextHandleFunction = (
req: IncomingMessage,
res: http.ServerResponse,
next: NextFunction,
) => void
export type ErrorHandleFunction = (
err: any,
req: IncomingMessage,
res: http.ServerResponse,
next: NextFunction,
) => void That means the types would've restricted any middlewares added to |
I think my overall gut feeling with this is that we should migrate our playground/examples to polka where possible (especially in https://github.com/bluwy/create-vite-extra), but still keep connect in core (or could use a lighter fully-connect-compatible package). Middlewares play a vital part in many Vite plugins and frameworks, and we need to be careful changing it & make sure there's a clear benefit if it needs to break something. I'm still open to be persuaded if polka in core is the right choice though. (Sorry for causing some uncertain work on your end) |
I agree with Bjorn here. It is great to see what a polka based Vite core would look like though. For reference, there are other servers that integrate with Vite adapting the connect middlewares (fastify, hono), and a lot of frameworks will need to be reworked if we move away from connect. I think moving to a leaner connect is an interesting idea. For the long term, we probably need a way out of connect though once everyone moves out of the express ecosystem. |
you're right, the playground stuff uses express explicitly and thats where i had seen those extensions in that case, its as simple as not exposing the extended properties (its still just a server response/request under the hood. so this is just decided by what types we expose) as for the fact this could be a big breaking change - maybe. polka accepts connect-like middleware just like express and connect itself. i wouldn't expect many differences, if any. they all deal with node requests and responses the same, and they all order middleware the same afaict. i don't have anything left to do on this branch anyway - it builds and tests pass. so if you're not ready for it or don't want to move away from connect, feel free to close it or keep the branch for future reference somewhere. all good by me, was fun to do |
/ecosystem-ci run |
Let's do a ecosystem-ci run so we get some signals from downstream projects. |
📝 Ran ecosystem CI on
✅ histoire, laravel, quasar, unocss, vite-plugin-pwa, vite-plugin-react-swc, vitepress |
it looks like most of the failures are projects depending on in hindsight, it was probably a bad idea to have exposed that in vite's public interface as it locks us in to a specific server under the hood even if this doesn't land, we should probably work to move away from that some day. instead, exposing a wrapper or some such thing so the underlying server doesn't matter |
I wanted to put in a note of support for this PR. I filed an issue (#17421) a couple of months ago that this would solve, which is that I expect that the express isn't going anywhere in the JS community. With express 5 on the horizon, I think express will only be re-invigorated. Using polka is nice because its middleware is basically API-compatible with express, which is the de facto standard. While it would be a breaking change for Vite 6, I think it would be relatively easy for frameworks to move to. I don't expect we'd have to do much in SvelteKit besides update the reference to the connect type in two places (example). We have been using polka in SvelteKit since the beginning and have used express middlewares with polka in the past. I've always just been able to drop them in and have never had any issues at runtime with express/polka compatibility. And it goes the other way too, which is why we're able to use sirv with connect in Vite today. The only thing I've run into that is slightly annoying is that the middleware types don't quite match up and you'll often need a typecast to use an express middleware in polka (e.g. see lukeed/polka#173). If Vite 6 doesn't have breaking changes that would prevent it, we may simply release a minor of SvelteKit that extends SvelteKit's peerDep range to include both Vite 5 and 6 when Vite 6 is released. I expect that we would still be able to do that with this PR and would be happy to help test and verify that as a measure of compatibility. |
We've discussed in the last meeting that it's not worth migrating to polka for now. Because we expose If we do a compat layer, the effort may be worth perhaps instead to add support for asynchronous middlewares in connect? Either by upstreaming or patching. And if we want to do a breaking change, it might be worth looking into proper |
Yeah I agree We would need a compatibility layer to hide the internal choice of web server I think we should aim to do that one way or another. Right now we have vendor lock in with connect because we expose their types in our own. Really, we should be exposing our own types that hide away what underlying server we use Bunch of effort but should happen imo |
I'll close this for now as we're not going with this direction for now, but the discussion has been valuable here. Thanks for the PR! |
Migrates from connect to polka, a much lighter and faster middleware server/router.
Possibly breaking changes
app.middlewares
will now be aPolka
instance of course, which has different methods and behaviour to an express instanceResponse
a middleware receives will be a regular nodeServerResponse
near enough, i.e. nostatus(num)
method etc (useres.statusCode
instead)Request
a middleware receives will be a polkaRequest
which has a similar shape to express', but may differ slightly