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

Ensure consistent server route handling by always starting managerBuilder before previewBuilder #19406

Merged
merged 2 commits into from
Oct 12, 2022

Conversation

JReinhold
Copy link
Contributor

@JReinhold JReinhold commented Oct 8, 2022

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');
          });
        },
      },
    ],
  }),
  1. yarn storybook in the Sandbox
  2. 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.
  3. git checkout origin/next
  4. Start Storybook again
  5. 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 JReinhold added builder-vite build Internal-facing build tooling & test updates labels Oct 8, 2022
@JReinhold
Copy link
Contributor Author

JReinhold commented Oct 8, 2022

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

@JReinhold JReinhold marked this pull request as ready for review October 8, 2022 22:25
Copy link
Member

@IanVS IanVS left a comment

Choose a reason for hiding this comment

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

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.

@shilman shilman added bug and removed build Internal-facing build tooling & test updates labels Oct 9, 2022
@JReinhold
Copy link
Contributor Author

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

tmeasday commented Oct 9, 2022

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

IanVS commented Oct 10, 2022

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.

@JReinhold JReinhold self-assigned this Oct 11, 2022
@ndelangen ndelangen merged commit c3bb8b7 into next Oct 12, 2022
@ndelangen ndelangen deleted the jeppe/enforce-manager-builder-first branch October 12, 2022 07:17
@IanVS IanVS mentioned this pull request Oct 12, 2022
3 tasks
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.

5 participants