feat: formalize waitForRequestsIdle (experimental)

[patak-dev]

Description

We discussed with @ArnaudBarre about how to delay processing of generated CSS until all other source code has been processed in tools like UnoCSS and Tailwind. This is needed to avoid a flash of style changes.

This PR proposes a solution by reusing the crawl end finder for static imports we have been using for deps optimization. The issue is similar to discovering all imported dependencies before loading them in the browser to avoid the need of full-reloads.

The crawl end finder was only run on cold start. It has been extracted here out of the deps optimizer and is being always run on startup.

There is a new experimental server.delayUntilStaticImportsProcessed(id) function that can be called on the load hook to delay processing of a certain id

[stackblitz]

Run & review this pull request in StackBlitz Codeflow.

[bluwy]

Is there a reason we need a builtin API to support this usecase? Couldn't a workaround like this (related #6011) works instead? And until Rollup also supports a standard way for this, we can then implement that too? I know it was discussed in rollup/rollup#4985, but it sounds like the API hasn't been figured out, but the feature request is still relevant.

[patak-dev]

That workaround is using Vite internals. This PR gives Vuetify, tailwind, UnoCSS, and others a way to do it without risking a breaking change (I actually was thinking on removing _pendingRequests as part of the work of the Vite Environment API, thanks for the reminder that is now kind of a public API 😬), and in a robust way that has proven to work well for deps optimization where we had the same issue they are experiencing.
I think the solution in build time will not be something we can implement during dev because the focus here in getting something that works as best as possible without hurting the on demand nature of the server. We could end up with a different API for this though if rollup implements something like a shouldLoad(id, loadingQueue, processingQueue) hook to let projects order the loading queue and push CSS as the last ones to load. But I don't know how long it will take to develop this API in rollup, I think designing it in a way that doesn't end up being a z-index like fight isn't easy.

If we get an API in rollup we could also adapt to dev, we can deprecate the API proposed in this PR. We can keep this one as @experimental as a signal that it may be removed in a future major. But at least we give downstream projects a solution for the next major (or majors?).

[ArnaudBarre]

Wow so many array duplication and promise created, if everyone do that the performance impact on big codebases could be non negligeable. And maybe I'm missing something, but how does this hack works if two plugins do the same?

[patak-dev]

I think the promises and array overhead shouldn't be that bad, specially if you don't hold to the promises like we do in core. The implementation in Vuetify does looks like it could add up quickly though. Your other point is more important for this PR, because this only works if there is a global controller that knows every id that is awaiting. If not, you get locked and both parties will never resolve (or they will have to have a time out to bail out defeating the feature). This is a big reason to include this in core.

[KrisBraun]

I've tested this with @tailwind/vite and confirmed it solves the two issues we have:

1. We can now wait until we've processed all content files before generating the Tailwind CSS. This avoids extra rebuilds when the CSS was generated too early.
2. There is no longer a FOUC when CSS was generated before seeing all content.

[patak-dev]

I've tested this with @tailwind/vite and confirmed it solves the two issues we have:

Nice! Thanks for testing the PR 🙏🏼

[bluwy]

The Vuetify link would be the rough idea of a workaround and I'm sure it could be simplified to be more performant. If multiple plugins do the same, they'll cut off each other because of the timeout, so it won't cause a deadlock. Our implementation also has a fixed timeout so I don't think there's a difference.

Also, Vuetify doesn't do this hack anymore, it's something they did in the past but they removed a feature that needed this altogether. So I think this will only be for tailwind and unocss today.

If we want to support this API still, I don't mind merging. Just felt like it isn't the right abstraction to solve generic problems, rather a very-specific API to solve a single problem. Maybe renaming it as waitForRequestsSettled or waitForRequestsIdle helps clarify its usecase more (naming idea from playwright api).

[patak-dev]

The Vuetify link would be the rough idea of a workaround and I'm sure it could be simplified to be more performant. If multiple plugins do the same, they'll cut off each other because of the timeout, so it won't cause a deadlock. Our implementation also has a fixed timeout so I don't think there's a difference.

It is different because we are in control of what ids are being awaited. So we have a single central controller that avoids the deadlocks. When you call this function in plugin hooks, you should pass the id so everybody else will avoid waiting for it. This isn't possible when each project has their own waiting scheme.

Also, Vuetify doesn't do this hack anymore, it's something they did in the past but they removed a feature that needed this altogether. So I think this will only be for tailwind and unocss today.

And for us in the dependeny prebundler, but we are trying to move away from it too. I agree we should promote moving away from this kind of strategies as much as possible, but for projects like tailwind that is a hard ask at the moment (I don't know if it is even possible).

If we want to support this API still, I don't mind merging. Just felt like it isn't the right abstraction to solve generic problems, rather a very-specific API to solve a single problem. Maybe renaming it as waitForRequestsSettled or waitForRequestsIdle helps clarify its usecase more (naming idea from playwright api).

I renamed the function to waitForRequestsIdle, thanks for the suggestion! I also added docs including a warning as suggested by @sapphi-red in Discord.

[bluwy]

When you call this function in plugin hooks, you should pass the id so everybody else will avoid waiting for it. This isn't possible when each project has their own waiting scheme.

What I had in mind is that if the number of server._pendingRequests stays stagnant after a certain period, and you've iterated each requests and they all never resolve, then you can tell that the requests are "idle". So I think it is possible and have a reliable way to tell in that case.

But anyways, it seems like we'll move forward with it. My concern isn't that we should move away from these strategies though (I also did something like this for my projects), but my main concern was that the API doesn't feel like the right abstraction for the problem (less so now with the rename).

[sapphi-red]

I think the code looks good.
However, I don't really understand it and would appreciate clarification. Why does this API prevent FOUC from happening? Is there any difference in that respect from implementing it inside a plugin?

[patak-dev]

Why does this API prevent FOUC from happening?

Tailwind and UnoCSS creates the needed classes used in the app as it discovers them, and triggers CSS HMR events. When the CSS is loaded the first time, before all the source code has been processed it doesn't have all the content it should. Later on, when the CSS is re-generated, a HMR will populate the CSS with the proper classes and a flash of changed styles will be experienced. The tailwind plugin will solve this by calling server.waitForRequestsIdle(id) in the transform hook of any CSS that has the tailwind directive. The browser will request these CSS files normally, but the server wont answer until the promise is released when all static imports have been crawled. And so, the issue is avoided. It is the same problem we have with optimized dependencies, were we use this exact same function to wait until we discover all imported dependencies before releasing the pre-bundled deps, avoiding possible full-reloads.

Is there any difference in that respect from implementing it inside a plugin?

As far as I can see, we can't do that. We need a central store that everyone uses. We are already using this API in the deps optimizer. If someone else prevents a CSS from going through the pipeline, there will be a deadlock. We could avoid awaiting CSS on the optimizer, but then you have the same issue with what vuetify was doing for JS files. I think the key here is that we need a way to ignore all the ids that are awaiting. Here is an equivalent feature request for build time that we needed at one point (now it isn't needed as we are not doing deps optimization during build anymore):

• Delay loading a set of ids until all other files have been processed rollup/rollup#4985

Tailwind folks may explore some proposals to implement this feature in rollup in the future, because right now they are hacking their way to be able to wait before generating the CSS file.

[sapphi-red]

Thanks!

Why does this API prevent FOUC from happening?

[answer]

I see. IIUC this only prevent FOUC for CSR apps, right? The CSS file will be stalled and until that request resolves, the entire JS files won't be executed by the browser, and the HTML generation won't happen. If the HTML already exists (for example, when SSR is done), FOUC would still happen.

[patak-dev]

Yes, this is for CSR. But IIUC, in the SSR case, a plugin will be able to see all the source included in the HTML so it can already use that to generate the full CSS before the browser requests it. So there will not be FOUC either there.

[sapphi-red]

Ah, yeah, that's true. I've understood. 👍

[ZerdoX-x]

Very sorry to ping all participants, but I spent about 5 hours debugging vite and found out that this PR introduced a bug for me. After upgrading from v5.2.0-beta.0 to v5.2.0-beta.1 (and all next versions) I got very strange behavior: dev server doesn't respond to several file requests (e.g .vite/deps/esm-env.js?v=846d6b91), page is loading infinitely and F5 doesn't help. I have recovered all removed logic (just all red lines from git diff), without touching new logic, and my server is now responding fine.

I have no idea what the code does, but the entry point and cause of the problem is change in transformRequest.ts

Could someone help me debugging this change? Or re-review the changes? Maybe I stumbled upon some rare case? I tried to reproduce the issue in freshly created project, no luck.

I have created thread 1220740873273475162 on discord, you could reach me out there, and see screenshot of the problem.

Thank you for your attention!

diff of my "fix"

diff --git a/packages/vite/src/node/optimizer/index.ts b/packages/vite/src/node/optimizer/index.ts
index 4c98a289c..591ea0cc8 100644
--- a/packages/vite/src/node/optimizer/index.ts
+++ b/packages/vite/src/node/optimizer/index.ts
@@ -59,6 +59,7 @@ export interface DepsOptimizer {
   isOptimizedDepFile: (id: string) => boolean
   isOptimizedDepUrl: (url: string) => boolean
   getOptimizedDepId: (depInfo: OptimizedDepInfo) => string
+  delayDepsOptimizerUntil: (id: string, done: () => Promise<any>) => void
 
   close: () => Promise<void>
 
diff --git a/packages/vite/src/node/optimizer/optimizer.ts b/packages/vite/src/node/optimizer/optimizer.ts
index 096d0bef2..75ac23d43 100644
--- a/packages/vite/src/node/optimizer/optimizer.ts
+++ b/packages/vite/src/node/optimizer/optimizer.ts
@@ -105,6 +105,7 @@ async function createDepsOptimizer(
     isOptimizedDepUrl: createIsOptimizedDepUrl(config),
     getOptimizedDepId: (depInfo: OptimizedDepInfo) =>
       `${depInfo.file}?v=${depInfo.browserHash}`,
+    delayDepsOptimizerUntil,
     close,
     options,
   }
@@ -166,8 +167,10 @@ async function createDepsOptimizer(
   // from the first request before resolving to minimize full page reloads.
   // On warm start or after the first optimization is run, we use a simpler
   // debounce strategy each time a new dep is discovered.
+  let crawlEndFinder: CrawlEndFinder | undefined
   let waitingForCrawlEnd = false
   if (!cachedMetadata) {
+    crawlEndFinder = setupOnCrawlEnd(onCrawlEnd)
     server._onCrawlEnd(onCrawlEnd)
     waitingForCrawlEnd = true
   }
@@ -754,6 +757,69 @@ async function createDepsOptimizer(
       debouncedProcessing(0)
     }
   }
+  function delayDepsOptimizerUntil(id: string, done: () => Promise<any>) {
+    if (crawlEndFinder && !depsOptimizer.isOptimizedDepFile(id)) {
+      crawlEndFinder.delayDepsOptimizerUntil(id, done)
+    }
+  }
+}
+
+const callCrawlEndIfIdleAfterMs = 50
+
+interface CrawlEndFinder {
+  delayDepsOptimizerUntil: (id: string, done: () => Promise<any>) => void
+  cancel: () => void
+}
+function setupOnCrawlEnd(onCrawlEnd: () => void): CrawlEndFinder {
+  const registeredIds = new Set<string>()
+  const seenIds = new Set<string>()
+  let timeoutHandle: NodeJS.Timeout | undefined
+
+  let cancelled = false
+  function cancel() {
+    cancelled = true
+  }
+
+  let crawlEndCalled = false
+  function callOnCrawlEnd() {
+    if (!cancelled && !crawlEndCalled) {
+      crawlEndCalled = true
+      onCrawlEnd()
+    }
+  }
+
+  function delayDepsOptimizerUntil(id: string, done: () => Promise<any>): void {
+    if (!seenIds.has(id)) {
+      seenIds.add(id)
+      registeredIds.add(id)
+      done()
+        .catch(() => {})
+        .finally(() => markIdAsDone(id))
+    }
+  }
+  function markIdAsDone(id: string): void {
+    registeredIds.delete(id)
+    checkIfCrawlEndAfterTimeout()
+  }
+
+  function checkIfCrawlEndAfterTimeout() {
+    if (cancelled || registeredIds.size > 0) return
+
+    if (timeoutHandle) clearTimeout(timeoutHandle)
+    timeoutHandle = setTimeout(
+      callOnCrawlEndWhenIdle,
+      callCrawlEndIfIdleAfterMs,
+    )
+  }
+  async function callOnCrawlEndWhenIdle() {
+    if (cancelled || registeredIds.size > 0) return
+    callOnCrawlEnd()
+  }
+
+  return {
+    delayDepsOptimizerUntil,
+    cancel,
+  }
 }
 
 async function createDevSsrDepsOptimizer(
@@ -776,6 +842,7 @@ async function createDevSsrDepsOptimizer(
     // noop, there is no scanning during dev SSR
     // the optimizer blocks the server start
     run: () => {},
+   delayDepsOptimizerUntil: (id: string, done: () => Promise<any>) => {},
 
     close: async () => {},
     options: config.ssr.optimizeDeps,
diff --git a/packages/vite/src/node/server/transformRequest.ts b/packages/vite/src/node/server/transformRequest.ts
index 8341f14a7..c6fc76d97 100644
--- a/packages/vite/src/node/server/transformRequest.ts
+++ b/packages/vite/src/node/server/transformRequest.ts
@@ -180,10 +180,16 @@ async function doTransform(
     module,
     resolved,
   )
-
+  getDepsOptimizer(config, ssr)?.delayDepsOptimizerUntil(id, () => {
+    console.log({ result, id, ssr })
+    return result
+  })
   const depsOptimizer = getDepsOptimizer(config, ssr)
   if (!depsOptimizer?.isOptimizedDepFile(id)) {
-    server._registerRequestProcessing(id, () => result)
+    server._registerRequestProcessing(id, () => {
+      console.log({ id, result })
+      return result
+    })
   }
 
   return result

[patak-dev]

@ZerdoX-x I reviewed the PR again, and I can't find how it could be causing your issue. Please try to create a minimal reproduction and open a new issue so we can track and fix this problem. Thanks!

[knd775]

I can confirm that there is an issue. Running git bisect and pnpm link to find which commit causes things to break points to this PR. Works before, doesn't work after. A minimal reproduction has proven quite difficult as we have no idea what is causing it. A simple sveltekit starter app doesn't have this issue.

[ZerdoX-x]

@knd775 i have managed to reproduce the issue with inlang's paraglide vite. i'll post it later and ping here

[ZerdoX-x]

https://git.sr.ht/~zerdox/paraglide-js-adapter-sveltekit_incompatibility_vite-5.2.0-beta.1

Here you are. I'll open issues in both vite and opral (inlang) repos

@knd775 do you use paraglide? could you list vite plugins you use? try to drop them one by one in your project to see the one causing your issue. more info will let us know affected plugins

let's move our discussion to the issue linked below