vite icon indicating copy to clipboard operation
vite copied to clipboard

feat: migrate from connect to polka

Open 43081j opened this issue 1 year ago • 11 comments

Migrates from connect to polka, a much lighter and faster middleware server/router.

Possibly breaking changes

  • app.middlewares will now be a Polka instance of course, which has different methods and behaviour to an express instance
  • polka has a single error handler rather than passing a middleware to do it
  • the Response a middleware receives will be a regular node ServerResponse near enough, i.e. no status(num) method etc (use res.statusCode instead)
  • the Request a middleware receives will be a polka Request which has a similar shape to express', but may differ slightly

43081j avatar Jun 26 '24 11:06 43081j

Review PR in StackBlitz Codeflow Run & review this pull request in StackBlitz Codeflow.

(I noticed you edited away some previous concerns you had. Do they still apply now?)

moving to polka 1.x solved all of these (1.x orders middleware the same way express/connect do)

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

connect/express cause the same problem but with Response. it wouldn't surprise me if there's middleware in the wild that assumes res.status(n) exists for example

we could just dumb the type down if we don't want to publicly support the extensions

43081j avatar Jul 30 '24 09:07 43081j

Good to know that polka v1 solves them 👍

connect/express cause the same problem but with Response

According to the previous connect types, I don't think they do? req is extended with one property, while res is plain http.ServerResponse:

  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 viteServer.middlewares to not be able to use res.status() and such.

bluwy avatar Jul 30 '24 12:07 bluwy

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)

bluwy avatar Jul 30 '24 13:07 bluwy

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.

patak-dev avatar Jul 30 '24 14:07 patak-dev

According to the previous connect types, I don't think they do? req is extended with one property, while res is plain http.ServerResponse

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

43081j avatar Jul 30 '24 16:07 43081j

/ecosystem-ci run

patak-dev avatar Jul 30 '24 17:07 patak-dev

Let's do a ecosystem-ci run so we get some signals from downstream projects.

patak-dev avatar Jul 30 '24 17:07 patak-dev

📝 Ran ecosystem CI on 7c6ab6f: Open

suite result latest scheduled
analogjs :x: failure :white_check_mark: success
astro :x: failure :x: failure
ladle :x: failure :white_check_mark: success
marko :x: failure :white_check_mark: success
nuxt :x: failure :x: failure
previewjs :x: failure :white_check_mark: success
qwik :x: failure :x: failure
rakkas :x: failure :white_check_mark: success
remix :x: failure :white_check_mark: success
sveltekit :x: failure :white_check_mark: success
vike :x: failure :x: failure
vite-plugin-react :x: failure :white_check_mark: success
vite-plugin-react-pages :x: failure :x: failure
vite-plugin-svelte :x: failure :white_check_mark: success
vite-plugin-vue :x: failure :white_check_mark: success
vite-setup-catalogue :x: failure :white_check_mark: success
vitest :x: failure :x: failure

:white_check_mark: histoire, laravel, quasar, unocss, vite-plugin-pwa, vite-plugin-react-swc, vitepress

vite-ecosystem-ci[bot] avatar Jul 30 '24 17:07 vite-ecosystem-ci[bot]

it looks like most of the failures are projects depending on Connect being exported

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

43081j avatar Jul 30 '24 22:07 43081j

I wanted to put in a note of support for this PR. I filed an issue (https://github.com/vitejs/vite/issues/17421) a couple of months ago that this would solve, which is that connect doesn't support async middlewares. While you can write your own middlewares that work with connect in an async fashion, I expect most people are doing it wrong. And even when you can do it, it's cumbersome because it's a fair amount more code and you need to silence warnings from typescript-eslint.

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 https://github.com/lukeed/polka/issues/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.

benmccann avatar Jul 31 '24 22:07 benmccann

We've discussed in the last meeting that it's not worth migrating to polka for now. Because we expose server.middlewares and plugins/frameworks have access to that, it'll break a big portion of the ecosystem without a compat layer.

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 Request / Response support too so we only break once.

bluwy avatar Oct 23 '24 14:10 bluwy

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

43081j avatar Oct 24 '24 08:10 43081j

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!

bluwy avatar Oct 31 '24 09:10 bluwy