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

Explore replacing express with fastify in dev server #17971

Open
benatshippabo opened this issue Apr 15, 2022 · 18 comments
Open

Explore replacing express with fastify in dev server #17971

benatshippabo opened this issue Apr 15, 2022 · 18 comments

Comments

@benatshippabo
Copy link

benatshippabo commented Apr 15, 2022

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

IanVS commented Apr 22, 2022

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. webpack/webpack-dev-server#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
Copy link
Member

shilman commented Apr 22, 2022

@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
Copy link
Member

IanVS commented Apr 22, 2022

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

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

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

@IanVS
Copy link
Member

IanVS commented Apr 22, 2022

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

benatshippabo commented Apr 22, 2022

@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

Areas of interest

Middlewares

app.use((req, res, next) => {
res.header('Access-Control-Allow-Origin', '*');
res.header('Access-Control-Allow-Headers', 'Origin, X-Requested-With, Content-Type, Accept');
// These headers are required to enable SharedArrayBuffer
// https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/SharedArrayBuffer
next();
});

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

router.use(targetEndpoint, express.static(staticPath, { index: false }));

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

Standard routing

router.get('/progress', (request: Request, response: Response) => {
let closed = false;
const close = () => {
closed = true;
response.end();
};
response.on('close', close);
if (closed || response.writableEnded) return;
response.setHeader('Cache-Control', 'no-cache');
response.setHeader('Content-Type', 'text/event-stream');
response.setHeader('Connection', 'keep-alive');
response.flushHeaders();
reportProgress = (progress: any) => {
if (closed || response.writableEnded) return;
response.write(`data: ${JSON.stringify(progress)}\n\n`);
response.flush();
if (progress.value === 1) close();
};
});

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

Passing our server to @storybook/builder-*

const preview = options.ignorePreview
? Promise.resolve()
: previewBuilder.start({
startTime,
options,
router,
server,
});
const manager = managerBuilder.start({
startTime,
options,
router,
server,
});

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

IanVS commented Apr 23, 2022

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

IanVS commented Apr 24, 2022

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

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

IanVS commented Apr 27, 2022

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

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

@IanVS
Copy link
Member

IanVS commented Jun 16, 2022

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 gajus/fastify-webpack-hot#3 at the moment. It's too early to tell if there are performance gains yet.

@IanVS IanVS changed the title Serve content over http2 when https is enabled Explore replacing express with fastify in dev server Jun 16, 2022
@IanVS
Copy link
Member

IanVS commented Jul 27, 2022

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 egoist/tsup#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
Copy link
Member

@IanVS Can I assist with that PR some time?

@IanVS
Copy link
Member

IanVS commented Jan 17, 2023

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

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

any updates on this?

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

No branches or pull requests

5 participants