What I did

Migrates from express to polka as the internal web server.

Work in progress

Still WIP as there's some leftover changes from the old connect branch we can probably discard/revert now.

Checklist for Contributors

Testing

The changes in this PR are covered in the following automated tests:

[ ] stories
[ ] unit tests
[ ] integration tests
[ ] end-to-end tests

Manual testing

N/A yet

Documentation

[ ] Add or update documentation reflecting your changes
[ ] If you are deprecating/removing a feature, make sure to update MIGRATION.MD

Checklist for Maintainers

[ ] When this PR is ready for testing, make sure to add ci:normal, ci:merged or ci:daily GH label to it to run a specific set of sandboxes. The particular set of sandboxes can be found in code/lib/cli/src/sandbox-templates.ts
[ ] Make sure this PR contains one of the labels below:

Available labels
- `bug`: Internal changes that fixes incorrect behavior. - `maintenance`: User-facing maintenance tasks. - `dependencies`: Upgrading (sometimes downgrading) dependencies. - `build`: Internal-facing build tooling & test updates. Will not show up in release changelog. - `cleanup`: Minor cleanup style change. Will not show up in release changelog. - `documentation`: Documentation **only** changes. Will not show up in release changelog. - `feature request`: Introducing a new feature. - `BREAKING CHANGE`: Changes that break compatibility in some way with current major version. - `other`: Changes that don't fit in the above categories.

🦋 Canary release

This pull request has been released as version 0.0.0-pr-29230-sha-6c3a0bb1. Try it out in a new sandbox by running npx storybook@0.0.0-pr-29230-sha-6c3a0bb1 sandbox or in an existing project with npx storybook@0.0.0-pr-29230-sha-6c3a0bb1 upgrade.

More information

| | | | --- | --- | | **Published version** | [`0.0.0-pr-29230-sha-6c3a0bb1`](https://npmjs.com/package/storybook/v/0.0.0-pr-29230-sha-6c3a0bb1) | | **Triggered by** | @JReinhold | | **Repository** | [43081j/storybook](https://github.com/43081j/storybook) | | **Branch** | [`without-express-polka`](https://github.com/43081j/storybook/tree/without-express-polka) | | **Commit** | [`6c3a0bb1`](https://github.com/43081j/storybook/commit/6c3a0bb1eee028f7a5fccc86194ac333a36f0280) | | **Datetime** | Tue Oct 8 09:18:38 UTC 2024 (`1728379118`) | | **Workflow run** | [11232403641](https://github.com/storybookjs/storybook/actions/runs/11232403641) | To request a new release of this pull request, mention the `@storybookjs/core` team. _core team members can create a new canary release [here](https://github.com/storybookjs/storybook/actions/workflows/canary-release-pr.yml) or locally with `gh workflow run --repo storybookjs/storybook canary-release-pr.yml --field pr=29230`_

name	before	after	diff	z	%
createSize	0 B	0 B	0 B	-	-
generateSize	78.7 MB	78.7 MB	2.44 kB	1.12	0%
initSize	150 MB	149 MB	-1.29 MB	-3.27	-0.9%
diffSize	71.8 MB	70.5 MB	-1.29 MB	-2.87	-1.8%
buildSize	6.77 MB	6.77 MB	0 B	-0.41	0%
buildSbAddonsSize	1.5 MB	1.5 MB	0 B	-0.42	0%
buildSbCommonSize	195 kB	195 kB	0 B	-	0%
buildSbManagerSize	1.83 MB	1.83 MB	0 B	-0.4	0%
buildSbPreviewSize	270 kB	270 kB	0 B	-0.42	0%
buildStaticSize	0 B	0 B	0 B	-	-
buildPrebuildSize	3.8 MB	3.8 MB	0 B	-0.41	0%
buildPreviewSize	2.97 MB	2.97 MB	0 B	-0.69	0%
testBuildSize	0 B	0 B	0 B	-	-
testBuildSbAddonsSize	0 B	0 B	0 B	-	-
testBuildSbCommonSize	0 B	0 B	0 B	-	-
testBuildSbManagerSize	0 B	0 B	0 B	-	-
testBuildSbPreviewSize	0 B	0 B	0 B	-	-
testBuildStaticSize	0 B	0 B	0 B	-	-
testBuildPrebuildSize	0 B	0 B	0 B	-	-
testBuildPreviewSize	0 B	0 B	0 B	-	-

name	before	after	diff	z	%
createTime	18.9s	6.7s	-12s -209ms	-1.33	🔰-181.9%
generateTime	19.8s	23.1s	3.3s	0.74	14.2%
initTime	13.9s	14.2s	332ms	-0.47	2.3%
buildTime	11.1s	10.8s	-259ms	1.12	-2.4%
testBuildTime	0ms	0ms	0ms	-	-
devPreviewResponsive	7s	6.5s	-475ms	-0.62	-7.2%
devManagerResponsive	4.6s	4.2s	-323ms	-0.61	-7.5%
devManagerHeaderVisible	576ms	613ms	37ms	-0.1	6%
devManagerIndexVisible	604ms	653ms	49ms	-0.06	7.5%
devStoryVisibleUncached	658ms	1s	349ms	-0.57	34.7%
devStoryVisible	611ms	645ms	34ms	-0.13	5.3%
devAutodocsVisible	479ms	579ms	100ms	0.39	17.3%
devMDXVisible	528ms	533ms	5ms	-0.18	0.9%
buildManagerHeaderVisible	589ms	562ms	-27ms	-0.16	-4.8%
buildManagerIndexVisible	632ms	622ms	-10ms	0.06	-1.6%
buildStoryVisible	630ms	623ms	-7ms	-0.1	-1.1%
buildAutodocsVisible	475ms	484ms	9ms	-0.41	1.9%
buildMDXVisible	494ms	527ms	33ms	-0.1	6.3%

Greptile Summary

This PR migrates Storybook's internal web server from Express to Polka, aiming to reduce transitive dependencies and potentially improve performance.

Replaced Express with Polka in code/core/src/core-server/dev-server.ts, updating server initialization and middleware usage
Updated static file serving to use sirv instead of express.static in multiple files
Removed Express-specific types and methods, replacing them with more generic alternatives across the codebase
Modified package.json files to remove Express dependencies and add Polka-related packages
Adjusted response handling in various files to be compatible with Polka's API

nx-cloud[bot] commented 1 month ago

☁️ Nx Cloud Report

CI is running/has finished running commands for commit 6c3a0bb1eee028f7a5fccc86194ac333a36f0280. As they complete they will appear below. Click to see the status, the terminal output, and the build insights.

📂 See all runs for this CI Pipeline Execution

✅ Successfully ran 1 target

- [`nx run-many -t build --parallel=3`](https://cloud.nx.app/runs/UYB4OCTQst?utm_source=pull-request&utm_medium=comment)

Sent with 💌 from NxCloud.

43081j commented 1 month ago

@JReinhold this is where I got to with polka

I may not have much time over the next few days, so if you do get time, feel free to use and push to this branch

few things to note:

it may be possible to rename app back to router or whatever it was before, now that this uses polka
the places where we use sirv need manually testing
a few formatting changes are in here. I think they're right since, if I try revert them, the lint-staging process will just add them back (but they're in files I haven't otherwise touched)
haven't checked but I think the public interface exposes Polka.Polka which isn't ideal. maybe we should have a wrapper or dumbed down interface around it which we ship (so we're not tied into polka)

JReinhold commented 1 month ago

Thank you! 🙏

I'll make sure it gets over the finish line next week.

JReinhold commented 1 month ago

Todo

[x] Don't expose Polka types
[x] Test the local dev server on Windows
[x] Test static files
- [x] Windows
- [x] Paths outside of root should not be allowed
- [x] Favicon
- [x] Multiple staticDirs
[x] Test custom middlewares with .storybook/middleware.js
[x] Webpack
[x] Vite
[x] RSBuild
[x] Modern Web Dev
[x] Remove express from scripts

storybook-bot commented 1 month ago

Failed to publish canary version of this pull request, triggered by @JReinhold. See the failed workflow run at: https://github.com/storybookjs/storybook/actions/runs/11161442195

storybook-bot commented 1 month ago

Failed to publish canary version of this pull request, triggered by @JReinhold. See the failed workflow run at: https://github.com/storybookjs/storybook/actions/runs/11161893237

JReinhold commented 1 month ago

Couldn't test rsbuild because of https://github.com/rspack-contrib/storybook-rsbuild/issues/136, but looking at the source it doesn't look like they're depending on any express-specific behavior in the router of start():

https://github.com/rspack-contrib/storybook-rsbuild/blob/main/packages/builder-rsbuild/src/index.ts#L130-L193

Same story for Modern Web, they don't yet support Storybook 8 so I couldn't test it, but their start() code looks good too:

https://github.com/modernweb-dev/web/blob/master/packages/storybook-builder/src/index.ts#L60-L133