storybookjs / storybook

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

Explore replacing express with fastify in dev server #17971

Open benatshippabo opened 2 years ago

benatshippabo commented 2 years ago

Is your feature request related to a problem? Please describe Serving content via http2 is beneficial and greatly improves load times when serving many files. This is the case when using using a no-bundler such as the builder-vite plugin

When using builder-vite, file count can easily exceed 1k files. This causes load times to be 🐌 . We can alleviate this by enabling http2 when appropriate.

Describe the solution you'd like When running storybook with https enabled:

# optionally we can also autogenerate the selfsigned cert
# maybe take inspiration from the selfsigned package on npm
start-storybook --https --ssl-cert selfsigned.crt --ssl-key selfsigned.key

storybook assets are served via http2

Describe alternatives you've considered Accept http1.1 bottleneck

Are you able to assist to bring the feature to reality? yes, I can...

Additional context

IanVS commented 2 years ago

Oooo, I like this idea, in principle. Load times for vite projects in dev can indeed get quite large, especially if storyStoreV7 is not enabled (which helps by not loading all stories at once, just what is needed).

Unfortunately, it looks like express (which the storybook dev server uses) still does not support http/2. :(. I kind of would have assumed that http/2 support would be worked out by now. webpack-dev-server, which uses express, also has some challenges. https://github.com/webpack/webpack-dev-server/issues/1713.

Fastify seems to be the popular new server on the block, and has support (https://github.com/fastify/fastify/blob/main/docs/Reference/HTTP2.md). But, swapping out the dev server seems like a major change, and I'd want to hear from @shilman and others on the core team what they think before looking into it too much further.

shilman commented 2 years ago

@IanVS I'd propose we make the on-demand store standard in v7 and then see where we stand.

Alternatively if you wanted to prototype a solution and benchmark it against the v7 store, then we could decide whether the perf gain is worth potential compatibility issues that come with switching.

IanVS commented 2 years ago

@shilman that sounds like a good plan.

@benatshippabo, would you be interested in investigating what it might take to swap out express to something that supports http/2, like fastify? There is a compat tool for express, so it might be possible to make a phased transition.

benatshippabo commented 2 years ago

@benatshippabo, would you be interested in investigating what it might take to swap out express to something that supports http/2, like fastify? There is a compat tool for express, so it might be possible to make a phased transition.

Sounds good, I like the idea of switching to fastify as well. I will keep you posted

benatshippabo commented 2 years ago

Perhaps we should wait for v4 to be released https://medium.com/@fastifyjs/announcing-fastify-v4-release-candidate-2b6be6edf196

IanVS commented 2 years ago

Perhaps we should wait for v4 to be released

Up to you. I think as an exploration / spike, you could probably use the release candidate of V4 to see how much work it is to switch, and what benefits we might get from doing so. Then, if it makes sense to actually make the switch for real, we'd probably want to use a stable v4, but it might be released by then anyway.

benatshippabo commented 2 years ago

@IanVS migrating from express to fastify looks very plausible based on my investigation thus far. We would only have to change about eleven files:

Files using express ![image](https://user-images.githubusercontent.com/66273043/164815920-21318790-3831-4956-8e8e-cfce40ea4fc2.png)

Areas of interest

Middlewares

https://github.com/storybookjs/storybook/blob/4c569ddd5574b09c6108e5ee861ba58e85ba5a94/lib/core-server/src/dev-server.ts#L36-L42

Fastify ^3.0.0 no longer supports middleware out-of-the-box and requires either:

  1. the mentioned fastify-express (this plugin isn't meant to be a long term solution)
  2. middie (more preferrable).

Once we setup something like middie, middleware will remain mostly the same. middie does not pass a next function since fastify handles errors for us. But based on our usages this should pose no issues.

Static assets

https://github.com/storybookjs/storybook/blob/4c569ddd5574b09c6108e5ee861ba58e85ba5a94/lib/core-server/src/utils/server-statics.ts#L47

We would need to use fastify-static and convert it to use res.sendFile() instead of express.static(). This seems pretty straightforward.

Standard routing

https://github.com/storybookjs/storybook/blob/4c569ddd5574b09c6108e5ee861ba58e85ba5a94/lib/core-common/src/utils/progress-reporting.ts#L14-L34

This should be an easy migration using fastify instead of express here should yield the same results.

Passing our server to @storybook/builder-*

https://github.com/storybookjs/storybook/blob/4c569ddd5574b09c6108e5ee861ba58e85ba5a94/lib/core-server/src/dev-server.ts#L82-L96

This is where questions arise, I'm not completely sure on the compatibility of passing a fastify instance to our @storybook/builder-* packages. We may need to update those packages upon switching.

IanVS commented 2 years ago

Thanks for the research and for sharing it here. Here's the code we have for dealing with the router and server in @storybook/builder-vite:

https://github.com/storybookjs/builder-vite/blob/0071562652391baa38c4afaed1bfdf3caeb8b71f/packages/builder-vite/index.ts#L40-L51

We mock out the /progress endpoint (webpack specific), add a middleware, and provide the server to vite for it to perform HMR (vite seems like it will be fine with fastify, FWIW).

If necessary, we can make a breaking change and require a particular version of builder-vite to be used with newer versions of storybook if this change is made. The webpack builders live in the monorepo and could be updated in lock-step. I'm not aware of any other builders for use with storybook.

I'm glad to hear that you think this is doable. It seems like the next step might be to prototype the change with some code, right? I'll be happy to work on the vite builder side of things, and to test it out if you're able to update storybook in a fork. If that makes a big difference in load times, we can get some feedback from the rest of the team and go from there. Does that sound like a plan?

IanVS commented 2 years ago

Hmmm, I tested out http/2 with vite directly, apart from storybook, and from what I can tell, it makes very little difference, unfortunately. My app loads 450 modules on first page load, and the first load happens in about 4.5 seconds regardless of http/1.1 or http/2 (you can start vite with h2 using --https). That's not to say that switching to fastify wouldn't be a good idea, and I think it might still have small performance benefits, but I don't think it's going to be a silver bullet, unfortunately.

benatshippabo commented 2 years ago

Thanks for the research and for sharing it here. Here's the code we have for dealing with the router and server in @storybook/builder-vite:

https://github.com/storybookjs/builder-vite/blob/0071562652391baa38c4afaed1bfdf3caeb8b71f/packages/builder-vite/index.ts#L40-L51

We mock out the /progress endpoint (webpack specific), add a middleware, and provide the server to vite for it to perform HMR (vite seems like it will be fine with fastify, FWIW).

If necessary, we can make a breaking change and require a particular version of builder-vite to be used with newer versions of storybook if this change is made. The webpack builders live in the monorepo and could be updated in lock-step. I'm not aware of any other builders for use with storybook.

I'm glad to hear that you think this is doable. It seems like the next step might be to prototype the change with some code, right? I'll be happy to work on the vite builder side of things, and to test it out if you're able to update storybook in a fork. If that makes a big difference in load times, we can get some feedback from the rest of the team and go from there. Does that sound like a plan?

Yeah sounds good, I will probably work on making a pull request that swaps out express for fastify over the next month or so.

Hmmm, I tested out http/2 with vite directly, apart from storybook, and from what I can tell, it makes very little difference, unfortunately. My app loads 450 modules on first page load, and the first load happens in about 4.5 seconds regardless of http/1.1 or http/2 (you can start vite with h2 using --https). That's not to say that switching to fastify wouldn't be a good idea, and I think it might still have small performance benefits, but I don't think it's going to be a silver bullet, unfortunately.

That is unfortunate news. I'll still keep working on this and see if it goes anywhere. I will keep you post on any updates @IanVS

IanVS commented 2 years ago

Awesome, thanks! I think if nothing else, it would set us up well for http3/quic, which does appear to have real perf benefits and seems to be getting some activity in node. And fastify will likely add support much faster than express, given the relative pace of development between the two. So yeah, in my opinion this is a good exploration regardless of http2 perf.

benatshippabo commented 2 years ago

Just a quick update, I've been swamped with other tasks recently. If anyone wants to pick up the work, feel free.

IanVS commented 2 years ago

I've made some progress on a spike. I can load up the manager in a browser with fastify (and without using fastify-express), but am a little bit stuck on https://github.com/gajus/fastify-webpack-hot/issues/3 at the moment. It's too early to tell if there are performance gains yet.

IanVS commented 2 years ago

Status update: I had fastify working briefly, before rebasing onto the latest next which now uses tsup, which I'm having some problems with. I've pushed up a WIP branch: https://github.com/storybookjs/storybook/tree/fastify. There's more that needs to be done, but until I can figure out how to deal with errors I'm getting from https://github.com/egoist/tsup/issues/14, I'm a bit stuck. The fastify-webpack-hot issue turned out to be solved, though, since in the new branch we're not running multiple versions of webpack at the same time, now that the manager is pre-bundled.

ndelangen commented 1 year ago

@IanVS Can I assist with that PR some time?

IanVS commented 1 year ago

Yeah that'd be great. I was going to wait until the push for 7.0 was over before I picked it back up, maybe as a 7.1 effort. Are you looking for something to do? :-D

ndelangen commented 1 year ago

Hahaha, no I'm no looking for more work, just hoping to assist where I can.

7.1 seems like the right target for this to me.

kamikazePT commented 10 months ago

any updates on this?