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

Core: Migrate from express to polka #29230

Merged
merged 43 commits into from
Oct 8, 2024

Conversation

43081j
Copy link
Contributor

@43081j 43081j commented Sep 27, 2024

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-6c3a0bb1. Try it out in a new sandbox by running npx [email protected] sandbox or in an existing project with npx [email protected] upgrade.

More information
Published version 0.0.0-pr-29230-sha-6c3a0bb1
Triggered by @JReinhold
Repository 43081j/storybook
Branch without-express-polka
Commit 6c3a0bb1
Datetime Tue Oct 8 09:18:38 UTC 2024 (1728379118)
Workflow run 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 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

43081j and others added 21 commits May 6, 2024 23:27
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.
Copy link

nx-cloud bot commented Sep 27, 2024

☁️ Nx Cloud Report

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

Sent with 💌 from NxCloud.

@43081j
Copy link
Contributor Author

43081j commented Sep 27, 2024

@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 JReinhold self-assigned this Sep 27, 2024
@JReinhold
Copy link
Contributor

Thank you! 🙏

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

@JReinhold JReinhold changed the title Migrate from express to polka Core: Migrate from express to polka Sep 30, 2024
@JReinhold JReinhold added the core label Sep 30, 2024
@storybook-bot
Copy link
Contributor

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

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

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

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

@greptile-apps greptile-apps bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

code/core/src/core-server/dev-server.ts Outdated Show resolved Hide resolved
code/core/src/core-server/dev-server.ts Outdated Show resolved Hide resolved
code/core/src/core-server/dev-server.ts Outdated Show resolved Hide resolved
code/core/src/core-server/dev-server.ts Show resolved Hide resolved
@shilman shilman mentioned this pull request Oct 6, 2024
8 tasks
@JReinhold JReinhold merged commit d16f880 into storybookjs:next Oct 8, 2024
55 of 57 checks passed
@ndelangen
Copy link
Member

🎉

@github-actions github-actions bot mentioned this pull request Oct 8, 2024
6 tasks
@43081j 43081j deleted the without-express-polka branch October 8, 2024 12:50
@shilman
Copy link
Member

shilman commented Oct 8, 2024

Simple time-to-first-render benchmarks for 8.4.0-alpha.4 and 8.4.0-alpha.5 indicate that the performance impact, if any, is negligible.

image

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

Successfully merging this pull request may close these issues.

Migrate from express to polka
5 participants