-
-
Notifications
You must be signed in to change notification settings - Fork 7.3k
fix(optimizer): ensure consistent browserHash between in-memory and persisted metadata #20609
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
base: main
Are you sure you want to change the base?
Conversation
…nd persisted metadata When the dev server restarts mid–asset waterfall and loads cached `_metadata.json`, inconsistent `browserHash` values can cause duplicate instances of the same dependency to be loaded (e.g. multiple React copies). This patch sets the `browserHash` eagerly in memory before resolving `scanProcessing`, ensuring both the in-memory metadata and the persisted metadata use the same value.
cc @bluwy is there anything I need to do to get this into triage? |
This comment was marked as duplicate.
This comment was marked as duplicate.
This comment was marked as duplicate.
This comment was marked as duplicate.
This comment was marked as duplicate.
This comment was marked as duplicate.
Hmm, I think Vite expects all assets used in the same page to be loaded from the same Vite server instance (i.e. Vite server is not restarted during loading a page and the assets).
Wouldn't this cause the WebSocket connection to disconnect and the client to call |
it makes sense that this would be an expectation, and I think there's a unique setup in Gadget that is exposing this issue where others probably haven't seen this ever
not necessarily; we see this happen the most frequently in react router SSR mode where the initial document is served and it loads some big modules like react and eventually it's hmr runtime which then loads the vite client (which then attempts to set up an hmr connection); if the server restarts while the hmr socket is trying to connect it can reject with:
in ordinary situations the vite client will start polling for a reconnect, however in the meantime a duplicate copy of react loads and the page load crashes with
In general do you agree that when vite first builds the optimized deps that the browser hash before and after restart should be consistent? It feels wrong that the browser hash in memory after the optimizer is finished doesn't match what it just wrote to disk |
This comment was marked as duplicate.
This comment was marked as duplicate.
I think it is better to be like that, but not necessarily have to be. So I'm not opposed unless it breaks other behaviors.
Hmm, that could happen. That said, unless there's a reason, I'll recommend loading the client before any other scripts so that the file changes during the initial load is sent to the browser properly. |
for (const dep of Object.keys(metadata.discovered)) { | ||
metadata.discovered[dep].browserHash = metadata.browserHash | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It looks like this would override metadata.discovered[dep].browserHash
that is set in addMissingDep
call above.
vite/packages/vite/src/node/optimizer/optimizer.ts
Lines 599 to 607 in e899bc7
// Adding a browserHash to this missing dependency that is unique to | |
// the current state of known + missing deps. If its optimizeDeps run | |
// doesn't alter the bundled files of previous known dependencies, | |
// we don't need a full reload and this browserHash will be kept | |
browserHash: getDiscoveredBrowserHash( | |
metadata.hash, | |
depsFromOptimizedDepInfo(metadata.optimized), | |
depsFromOptimizedDepInfo(metadata.discovered), | |
), |
I guess this would break the behavior written in the comment.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
taking a look; if there is behaviour that breaks I'll write a test to catch it
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
re-reading the code I think since this is the first deps build when !cachedMetadata
this override should be ok; at this point all the deps found during the initial scan processing are known and we can assign a stable hash (I think this is strongly signalled by the fact that this is the data that we will write down to disk)
it doesn't look like there is a test to cover the behaviour that was being addressed in #7378 and it sounds like setting one up would be tricky
@patak-dev any thoughts on this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I read the code several times and I think you're right; this assignment is fine.
But while thinking about that, I noticed that the fundamental problem we have is that the browserHash
for the discovered deps can be different from metadata.browserHash
which will be stored to the metadata file.
addMissingDep
generates browserHash
using getDiscoveredBrowserHash
and that hash will be used through out the same process for that dep. But that per-dep browserHash won't be saved to the metadata, only the top-level browserHash
is saved.
I think this means the browserHash
for any dep that was added by addMissingDep
would have a different hash when restarting the server. The other places addMissingDep
is called are:
missing = addMissingDep(id, resolved) addMissingDep(dep, result.metadata.optimized[dep].src!)
I think these also have to be fixed for the problem you have. That said, I'm not sure how we can fix the other places.
I'm fine with leaving the others for now if you prefer that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yah I don't see a great fix for that; but I will think about it more
if possible it'd be great to get this fix in and then I can see if I can find a solution for the others
yah that makes sense, unfortunately in our case we don't control the load order as it is determined by (1) the frontend framework (react-router v7 in this case) and then (2) our users that can write arbitrary code and our goal is to make the underlying tech as reliable as possible |
I'm going on vacation for a week, so tagging in my colleague @airhorns to address any comments while I'm away |
Just following up here -- happy to make any changes necessary or address comments
For this, you are right, but I think even if the vite client is the first script loaded on the page, we still can't guarantee that the vite server didn't reboot in between when the page was served and then the client is requested. I think in normal local operation it'd be incredibly unlucky, but in remote development contexts like Github codespaces / codesandbox / Gadget etc, I think the window of opportunity gets bigger. I am not incredibly familiar with Vite's internals, but AFAICT this shouldn't have much of a negative performance impact, and corrects the bug for these users, so is there a downside to including this fix? |
back from vacation now so I can respond to any feedback |
ping @sapphi-red think we are ok to merge this? |
I'd like another member to take a look. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not very familiar with the optimizer flow, but this seem fine to me.
For the new await depsOptimizer.scanProcessing
additions, can we add a comment about explaining why we're waiting for processing? I think it's to await for a stable browserHash as it could change?
@sapphi-red you feel comfortable to merge? |
My comment hasn't been addressed yet. And I'm sure sapphi will merge as soon as he feels ready, there's no need to push as the PR isn't stale yet. |
/ecosystem-ci run |
commit: |
This comment was marked as duplicate.
This comment was marked as duplicate.
📝 Ran ecosystem CI on
✅ astro, analogjs, nuxt, marko, histoire, one, ladle, react-router, rakkas, vite-environment-examples, qwik, storybook, vite-plugin-pwa, quasar, vike, vite-plugin-svelte, vite-plugin-vue, vite-plugin-react, waku, vitepress, vite-plugin-rsc, vuepress, vite-setup-catalogue, vitest, vite-plugin-cloudflare |
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm also not sure this will the complete fix for the explained use case, but this change looks okay on its own and as a start.
By searching ?v=
injection, I also saw this line for example. Is this code path not relevant or should also await scanProcessing
or is it already guaranteed somehow?
vite/packages/vite/src/node/plugins/resolve.ts
Lines 836 to 838 in d8169f0
const versionHash = depsOptimizer.metadata.browserHash | |
if (versionHash && isJsType) { | |
resolved = injectQuery(resolved, `v=${versionHash}`) |
hey folks, is there any thing I can do to help get this released? |
Btw, have you tested this patch on your platform? or does it require this to be published on npm? FYI, as a preview release, the package made from this PR can be installed like #20609 (comment) |
yah we've been running with this patch on top of version of |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think these two needs to be addressed.
For the new
await depsOptimizer.scanProcessing
additions, can we add a comment about explaining why we're waiting for processing? I think it's to await for a stable browserHash as it could change?
#20609 (review)
By searching
?v=
injection, I also saw this line for example. Is this code path not relevant or should also awaitscanProcessing
or is it already guaranteed somehow?
vite/packages/vite/src/node/plugins/resolve.ts
Lines 836 to 838 in d8169f0
const versionHash = depsOptimizer.metadata.browserHash if (versionHash && isJsType) { resolved = injectQuery(resolved, `v=${versionHash}`)
#20609 (review)
Description
This PR fixes an inconsistency in how
browserHash
values are set indepsOptimizer.metadata
during the initial dependency scan, which can cause duplicate module instances to be loaded in the browser when the Vite dev server restarts mid–asset waterfall.Background
At Gadget, we use Vite both for development (serving a React frontend) and for production builds. In development, each user works in an ephemeral environment (“sandbox”), which runs both:
For frontend-only changes, the Vite server stays running and uses HMR to push updates to the browser.
For backend changes, however, we restart the entire Fastify process — which also restarts the Vite dev server.
These backend restarts are cheap and frequent, so developers often trigger them while the frontend is still loading in the browser.
The Problem
When a restart happens during the initial page load, we can hit the following sequence:
depsOptimizer._metadata.json
) from scratch._metadata.json
from disk (written in step 1).Because
browserHash
in the in-memory metadata for the first server was not the same value eventually persisted to_metadata.json
, the same dependency can be served with two different browser hashes across the restart.If the dependency is React (or any library that maintains singleton state), this results in the browser having duplicate copies of the module, causing broken HMR, hydration errors, and subtle runtime issues.
Solution
This patch ensures that before resolving
scanProcessing
in the optimizer, we eagerly set thebrowserHash
on the in-memorymetadata
to the value that will later be written to disk.By doing this, both:
_metadata.json
)will use identical browser hashes for the same dependency, as long as we wait for the initial scan to complete before resolving dependencies.
Why This Matters