Ensure consistent server route handling by always starting managerBuilder before previewBuilder

[JReinhold]

This PR ensures that server middlewares are always loaded in a consistent order by changing the flow of how builders are started.

The Problem

In #19338 and #19280 we've run into an issue where SvelteKit's vite-plugin-svelte-kit takes over routing, meaning that when you try to open the Storybook you would instead get taken directly to your app. Our workaround was to disable vite-plugin-svelte-kit altogether.

The curious thing was that this problem wasn't present in 6.5, only in 7.0.

After debugging the server's router stack it turns out that in 7.0, the manager's handler for '/' is consistently at the end of the router stack while in 6.5 it's near the beginning. In the majority of cases this is not an issue, but if a Vite plugin handles '/' too and ends the response, it will overrule the manager's handler.
I suspect this is not only an issue with SvelteKit, it is just our first Vite framework that surfaces the problem.

This happens because we're currently starting the manager builder and preview builder simultaneously, and because they mutate the same router stack they are basically in a race to apply their route handling - leading to inconsistent race conditions. Handlers are invoked in the order they are applied in, so an early handler that ends the response will block any later handlers.

I'm still unsure why 7.0 builder-manager is always "later" than builder-vite, when 6.5 builder-webpack4 is always faster than builder-vite. It seems counter-intuitive that our new esbuild-based manager would get to adding the handler later than our old Webpack4-based manager would. But that it was happens. My hunch is maybe that builder-manager yields to the event loop more often than builder-webpack4, giving builder-vite more room to build, but I don't know if that even makes sense in practice.

The Solution

The solution is to start managerBuilder first, and not start previewBuilder until the manager is finished, ensuring that the route stack is always mutated in the same order - manager first.

Performance Caveat

In theory this could result in a performance penalty given that we are now sequentially building instead of building in parallel. But in practice I don't think that is noticeable given our builder-manager is pre-built with tsup (not addons though) and builds using esbuild. On my machine it's timed at 0.1 second from start to finish, but I admit it could be slower on older computers.

I see two alternative solutions addressing this performance problem. Both of them are more complex than this PR, which is why I haven't implemented them, but I'm open to suggestions.

1. Instead of giving each builder access to the root router, give them their own routers which the server adds in the correct order. Then it shouldn't matter which order the sub-handlers are added in as they will always be confined within their own router. This is only a theory I have based on the concept of "Nested Layers" (and based on what I see when logging router.stack). See this article.
2. Instead of passing a router to builders, make them return a list of handlers that the server then iterate over to apply to the router, and thus controlling the order of which they are applied. I'm sure this could work, but the API for builders becomes way worse IMO now that they can't just router.use(...) but have to maintain a list of routes and handlers to return.

Impact

I'm unsure if this has impact outside of the code I've tested, which is with react-vite/default-js and svelte-kit/skeleton-js sandboxes. I wouldn't consider this a breaking change that needs a note in MIGRATION.md because we're essentially reverting the middleware handling back to the order it was in 6.5. User's custom middlewares will still be applied before anything else as usual.

Additional notes

Now that the managers don't run in parallel there's really no reason to ever invoke the bail() function on a preview builder, since the manager builder will always be done by the time the preview builder starts. If my assumption here is correct I think we should remove bail from builder-vite and builder-webpack5. I'm not sure what to do with the Builder typings though, because I still think it makes sense for the builder-manager to have a bail so we can stop the esbuild watch mode with a failing preview builder.

How to test

This is quite cumbersome to test:

1. git checkout origin/jeppe/enforce-manager-builder-first
2. Create a Sandbox with Vite, eg. react-vite/default-js
3. In main.cjs add a testing plugin to viteFinal like this:

  viteFinal: (config) => ({
    ...config,
    optimizeDeps: { ...config.optimizeDeps, force: true },
    plugins: [
      ...config.plugins,
      {
        name: 'my-interceptor',
        configureServer(server) {
          server.middlewares.use('/', (req, res, next) => {
            console.log('Intercepting middleware called');
            res.status(418).end('Response from intercepting middleware');
          });
        },
      },
    ],
  }),

4. yarn storybook in the Sandbox
5. See that Storybook loads correctly, but in the Network tab see that the final responses have the 418 status code from the interceptor - meaning it exists in the stack.
6. git checkout origin/next
7. Start Storybook again
8. See that you don't get Storybook now, but instead a blank page with "Response from intercepting middleware". (Because the manager is never allowed to handle the route)

[JReinhold]

Failing tests don't seem to have anything to do with this PR.

[IanVS]

Great detective work here! And thanks for taking the time to detail your findings and alternatives in the description.

In terms of performance, I decided to test it out in my own react typescript app at work. The baseline (7.0.0-alpha.35) was 16.5s over three runs, with the manager reporting 16s and preview 17s.

With the changes here, the total startup time averaged to 16.75s over three runs, with the manager reporting 4.25s and preview 17s.

So, not only was the total time essentially unchanged, but the manager time was more accurate, which is nice to see and hopefully would prevent confusion.

Furthermore, I think it would be tricky to only have the preview register routes, since at least for vite, we pass the dev server to vite when we start it up, so that vite can run in "middleware mode."

All this to say, I think this is a good solution, and a nice find.

[JReinhold]

Thank you for testing it out @IanVS !

Can you help me understand these numbers?

With the changes here, the total startup time averaged to 16.75s over three runs, with the manager reporting 4.25s and preview 17s.

With the two builders running in sequence I'd suspect the total time to be a sum of the two - 21s - why is it still 17s?

[tmeasday]

One thing I'd add here is that (I believe) we are planning on precompiling the manager for at least the core + essential packages and including it in the npm bundle which should reduce the manager compile time significantly also.

[IanVS]

Yes, I think (though haven't proved yet) that the 4 seconds is a lie, maybe because the manager builder is a generator and is yielding between some steps?

The total time I calculated was from adding a console.time() as the first line of the storybookDevServer function, and then a console.timeEnd() just before the return statement.