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: migrate from connect to polka #17569

Closed
wants to merge 2 commits into from
Closed

Conversation

43081j
Copy link
Contributor

@43081j 43081j commented Jun 26, 2024

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

Copy link

stackblitz bot commented Jun 26, 2024

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

@43081j 43081j changed the title wip: migrate from connect to polka feat: migrate from connect to polka Jun 26, 2024
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.
@43081j 43081j marked this pull request as ready for review July 29, 2024 22:01
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).
Copy link
Member

@bluwy bluwy left a 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.

Comment on lines -1154 to +1155
middlewares.stack = newServer.middlewares.stack
middlewares.wares = newServer.middlewares.wares
Copy link
Member

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.

Copy link
Contributor Author

@43081j 43081j Jul 30, 2024

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

Copy link
Member

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)
Copy link
Member

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?

Copy link
Contributor Author

@43081j 43081j Jul 30, 2024

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

Copy link
Member

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.

@43081j
Copy link
Contributor Author

43081j commented Jul 30, 2024

(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

@bluwy
Copy link
Member

bluwy commented Jul 30, 2024

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
Copy link
Member

bluwy commented Jul 30, 2024

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)

@patak-dev
Copy link
Member

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.

@43081j
Copy link
Contributor Author

43081j commented Jul 30, 2024

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

@patak-dev
Copy link
Member

/ecosystem-ci run

@patak-dev
Copy link
Member

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

@vite-ecosystem-ci
Copy link

📝 Ran ecosystem CI on 7c6ab6f: Open

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

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

@43081j
Copy link
Contributor Author

43081j commented Jul 30, 2024

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

@benmccann benmccann mentioned this pull request Jul 31, 2024
4 tasks
@benmccann
Copy link
Collaborator

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

@bluwy bluwy added the p2-to-be-discussed Enhancement under consideration (priority) label Aug 15, 2024
@bluwy
Copy link
Member

bluwy commented Oct 23, 2024

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.

@43081j
Copy link
Contributor Author

43081j commented Oct 24, 2024

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

@bluwy
Copy link
Member

bluwy commented Oct 31, 2024

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 bluwy closed this Oct 31, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
p2-to-be-discussed Enhancement under consideration (priority)
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

4 participants