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

Migrate from express to polka #29083

Closed
Tracked by #29038
JReinhold opened this issue Sep 11, 2024 · 4 comments · Fixed by #29230
Closed
Tracked by #29038

Migrate from express to polka #29083

JReinhold opened this issue Sep 11, 2024 · 4 comments · Fixed by #29230
Assignees

Comments

@JReinhold
Copy link
Contributor

JReinhold commented Sep 11, 2024

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 #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
Copy link
Contributor

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
Copy link
Contributor Author

JReinhold commented Sep 25, 2024

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
Copy link
Contributor Author

(unrelated to above)

It can be worthwhile to read through the discussion about the same migration in Vite: vitejs/vite#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
Copy link
Contributor

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants