storybookjs / storybook

Storybook is the industry standard workshop for building, documenting, and testing UI components in isolation
https://storybook.js.org
MIT License
84.64k stars 9.31k forks source link

Migrate from `express` to `polka` #29083

Closed JReinhold closed 1 month ago

JReinhold commented 2 months ago

Express brings in a lot of transitive dependencies. We should try to mitigate that by migrating to polka. The next version has been used by many big projects like SvelteKit already, and is supposedly compatible with Express' middleware API.

express.static can be replaced by sirv, and compression with @polka/compression.

See https://github.com/storybookjs/storybook/pull/27045 for a prior attempt at this, migrating to connect instead.

Alternatively, if all else fails, we should investigate if we can remove @types/express as a dependency. It's not referenced anywhere in our code, and it brings in substantial dependencies, like @types/node: https://pkg-size.dev/@types/express

This migration should realistically only have an effect on builders. Besides testing our own builder-vite and builder-webpack5, we need to ensure it still works with:

tobiasdiez commented 1 month ago

May I ask why express/polka is needed in the first place? After a quick glance at the code, you don't seem to use any real router or runtime features, but "only" serve a couple of static files.

Would it be possible to serve them via the builder's dev server (e.g. vite dev server)? Currently, you run vite in middleware mode to be able to integrate it with the express server, but this mode is actually designed for ssr, which you really don't need here, right?

JReinhold commented 1 month ago

Great question @tobiasdiez, it has crossed our minds before, and I believe that @benmccann was the first to suggest this.

Basically changing the API to let the builders supply a server with middleware support, and then attach Storybook's (simplistic) middlewares. For Vite the obvious thing to do is just use Vite's dev server. Webpack doesn't have such a thing natively so the builder would have to include one like webpack-dev-server or polka.

The biggest problem here is that this would be a breaking change to all external builders. There are other builders other than the official Vite and Webpack ones, like rsbuild and Modern Web Dev, and we can't break them now. We could do this in a major, but it would require substantial effort the reverse the logic around, and I'm not sure it's worth the gains - at least not if we can get polka in which is tiny.

JReinhold commented 1 month ago

(unrelated to above)

It can be worthwhile to read through the discussion about the same migration in Vite: https://github.com/vitejs/vite/pull/17569

What I'm gathering from that is that currently Vite is exposing a lot of Express/Connect specific APIs and types instead of general middleware APIs, which makes it harder for them to migrate to polka because the internals changes (and thus breaks) slightly.

We have to ensure that we don't have the same problem, but I don't think so since polka's middleware API is compatible.

tobiasdiez commented 1 month ago

The biggest problem here is that this would be a breaking change to all external builders.

What about the following: One could have an optional createWebServer hook. Vite can then simply return it's webserver. In case a bundler doesn't provide the hook (or returns nothing from it) then storybook falls back to the same logic that it currently uses. Once most community bundler provide an implementation, this fallback can be removed.