Core: Migrate from `express` to `polka` #29230

43081j · 2024-09-27T12:01:45Z

Closes #29083

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-ffef33ce. Try it out in a new sandbox by running npx storybook@0.0.0-pr-29230-sha-ffef33ce sandbox or in an existing project with npx storybook@0.0.0-pr-29230-sha-ffef33ce upgrade.

More information


Published version	`0.0.0-pr-29230-sha-ffef33ce`
Triggered by	@JReinhold
Repository	43081j/storybook
Branch	`without-express-polka`
Commit	`ffef33ce`
Datetime	Thu Oct 3 12:43:50 UTC 2024 (`1727959430`)
Workflow run	11162208896

To request a new release of this pull request, mention the @storybookjs/core team.

core team members can create a new canary release here or locally with gh workflow run --repo storybookjs/storybook canary-release-pr.yml --field pr=29230

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

Migrates away from express, instead using connect and the underlying node server directly. Both express and connect support the same middleware structure, which means most middleware packages work for both and have remained the same in this change. A few notes: - Express' router has been replaced by basic connect mount points (though the two are not equivalent exactly) - Express static has been replaced by sirv This trims a large sub-tree of dependencies from our packages which express was pulling in. We in fact use very little of the functionality express gives us, only really making use of the connect-style API anyway.

Migrates from express to polka 1.x.

nx-cloud · 2024-09-27T12:05:55Z

☁️ Nx Cloud Report

CI is running/has finished running commands for commit 66eddc5. 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

Sent with 💌 from NxCloud.

43081j · 2024-09-27T12:16:34Z

@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)

code/builders/builder-vite/src/index.ts

JReinhold · 2024-09-27T12:43:49Z

Thank you! 🙏

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

…express-polka

JReinhold · 2024-10-01T12:22:45Z

storybook-bot · 2024-10-03T11:54:53Z

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 · 2024-10-03T12:26:01Z

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 · 2024-10-04T09:39:35Z

Couldn't test rsbuild because of rspack-contrib/storybook-rsbuild#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

…express-polka

greptile-apps

_{30 file(s) reviewed, 4 comment(s)}
_{Edit PR Review Bot Settings}

greptile-apps · 2024-10-04T10:46:53Z

code/core/src/core-server/dev-server.ts

-    options.presets.apply('core'),
-  ]);
+  const [server, core] = await Promise.all([getServer(options), options.presets.apply('core')]);
+  console.log('LOG: starting dev server');


style: Remove this console.log statement before merging

greptile-apps · 2024-10-04T10:46:54Z

code/core/src/core-server/dev-server.ts

+  // app.use((req, res, next) => {
+  //   console.log('LOG: ', { url: req.url, method: req.method });
+  //   next();
+  // });


style: Consider removing or uncommenting this debugging middleware

greptile-apps · 2024-10-04T10:46:55Z

code/core/src/core-server/dev-server.ts

+        app,
+        router: app, // back-compatability with express-based API


style: Passing both app and router: app might be redundant. Consider removing one

greptile-apps · 2024-10-04T10:46:56Z

code/core/src/core-server/dev-server.ts

@@ -143,7 +138,7 @@ export async function storybookDevServer(options: Options) {
  const previewResult = await previewStarted;

  // Now the preview has successfully started, we can count this as a 'dev' event.
-  doTelemetry(core, initializedStoryIndexGenerator, options);
+  doTelemetry(app, core, initializedStoryIndexGenerator, options);


logic: Update doTelemetry function to accept Polka app instead of Express

43081j and others added 21 commits May 6, 2024 23:27

fix

9b0b42a

Merge branch 'next' into pr/43081j/27045

c2261fd

fix linting

8129229

Merge branch 'next' into without-express

644582f

Merge branch 'next' into without-express

f1685e7

test: fix stories-json test assertions

ffa4f0b

fix(core-server): remove unused import

b92d83b

Merge branch 'next' into without-express

de5724b

Merge branch 'next' into without-express

480b07b

Merge branch 'next' into without-express

66d27f8

Merge branch 'next' into without-express

c515678

Merge branch 'next' into pr/43081j/27045

7832789

update branch with CPC work

2940276

Merge branch 'next' into without-express

e48040d

remove express types

88fbc1b

Merge branch 'next' into without-express

97d65b4

Merge branch 'next' into pr/43081j/27045

9e3cd4d

Merge branch 'next' into without-express

c18aedb

Merge branch 'next' into without-express-polka

4c2a2d4

feat: migrate from express to polka

841777d

Migrates from express to polka 1.x.

fix: revert a few changes

056a37c

JReinhold self-assigned this Sep 27, 2024

JReinhold reviewed Sep 27, 2024

View reviewed changes

code/builders/builder-vite/src/index.ts Outdated Show resolved Hide resolved

JReinhold changed the title ~~Migrate from express to polka~~ Core: Migrate from express to polka Sep 30, 2024

JReinhold added the core label Sep 30, 2024

listen for requests, cleanup

cce0f14

JReinhold added 2 commits October 1, 2024 13:49

comment update

b64c7cf

Merge branch 'next' of github.com:storybookjs/storybook into without-…

baa53b1

…express-polka

JReinhold added 6 commits October 2, 2024 09:05

remove custom content-type headers

4fd94e6

make polka a dev dep of builder-vite

0cafba6

prebundle polka, sirv

31ca3f7

revert to existing static serving logic

d85dc51

cleanup express dependencies

8f7f881

Don't expose Polka typings

2b4803e

fix middleware types

ffef33c

JReinhold mentioned this pull request Oct 4, 2024

The builder depends on internal use of handlebars that will be removed in Storybook 8.4 rspack-contrib/storybook-rsbuild#136

Closed

JReinhold added 2 commits October 4, 2024 11:46

only expose router to builders

b563eee

Merge branch 'next' of github.com:storybookjs/storybook into without-…

66eddc5

…express-polka

JReinhold marked this pull request as ready for review October 4, 2024 10:41

greptile-apps bot reviewed Oct 4, 2024

View reviewed changes

shilman mentioned this pull request Oct 6, 2024

bump express to v^4.21 #29079

Open

8 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Core: Migrate from `express` to `polka` #29230

Core: Migrate from `express` to `polka` #29230

43081j commented Sep 27, 2024 •

edited by greptile-apps bot

Loading

nx-cloud bot commented Sep 27, 2024 •

edited

Loading

43081j commented Sep 27, 2024

JReinhold commented Sep 27, 2024

JReinhold commented Oct 1, 2024 •

edited

Loading

storybook-bot commented Oct 3, 2024

storybook-bot commented Oct 3, 2024

JReinhold commented Oct 4, 2024

greptile-apps bot left a comment

greptile-apps bot Oct 4, 2024

greptile-apps bot Oct 4, 2024

greptile-apps bot Oct 4, 2024

greptile-apps bot Oct 4, 2024

		app,
		router: app, // back-compatability with express-based API

Core: Migrate from express to polka #29230

Are you sure you want to change the base?

Core: Migrate from express to polka #29230

Conversation

43081j commented Sep 27, 2024 • edited by greptile-apps bot Loading

What I did

Work in progress

Checklist for Contributors

Testing

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

Manual testing

Documentation

Checklist for Maintainers

🦋 Canary release

Greptile Summary

nx-cloud bot commented Sep 27, 2024 • edited Loading

☁️ Nx Cloud Report

43081j commented Sep 27, 2024

JReinhold commented Sep 27, 2024

JReinhold commented Oct 1, 2024 • edited Loading

Todo

storybook-bot commented Oct 3, 2024

storybook-bot commented Oct 3, 2024

JReinhold commented Oct 4, 2024

greptile-apps bot left a comment

Choose a reason for hiding this comment

greptile-apps bot Oct 4, 2024

Choose a reason for hiding this comment

greptile-apps bot Oct 4, 2024

Choose a reason for hiding this comment

greptile-apps bot Oct 4, 2024

Choose a reason for hiding this comment

greptile-apps bot Oct 4, 2024

Choose a reason for hiding this comment

Core: Migrate from `express` to `polka` #29230

Core: Migrate from `express` to `polka` #29230

43081j commented Sep 27, 2024 •

edited by greptile-apps bot

Loading

nx-cloud bot commented Sep 27, 2024 •

edited

Loading

JReinhold commented Oct 1, 2024 •

edited

Loading