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

fix: browser cache of newly discovered deps #7378

Merged
merged 2 commits into from Mar 19, 2022
Merged

Conversation

patak-dev
Copy link
Member

Description

I'm working to see if it is possible to have the scan phase be also non-blocking, and it is showcasing some issues with the current approach we started at #6758

An important observation is that we can only safely use 'max-age=31536000,immutable' for requests when we are using dependencies versioned with browserHash after a cached start, initial scan phase, or after discovery+full-reload. The browserHash that is computed for each newly discovered deps isn't fit for long-term caching with the approach previous to this PR. The issue is hard to manifest but it could happen when:

  1. There is a newly discovered dep_a that gets assigned bh_a
  2. It gets optimized and there aren't altered files (so it keeps bh_a and has a bundle o_a1)
  3. The cache is cleaned with --force on server restart
  4. There is a newly discovered dep_a that gets assigned bh_a
  5. There is another discovered dep_b that gets assigned bh_b
  6. Both gets optimized together (it can happen as there is a debounce)
  7. There are common chunks between them so dep_a ends up with bh_a but with bundle o_a2.
  8. The browser uses the cached version o_a1 because the version is bh_a

a. This PR solves this issue by including a server timestamp in the browserHash of newly discovered deps.

b. It also fixes versioning of chunks and dynamic imports, as they should get a stable hash, and they can be safely cached long term using the main browserHash

c. It also fixes an issue with the computing of alteredFiles that was broken due to paths, so it was always true.

Note: I think we should add a fileToDepInfoMap to speed up searching for depInfo from a file, which we are starting to do quite a bit.


What is the purpose of this pull request?

  • Bug fix
  • New Feature
  • Documentation update
  • Other

@patak-dev patak-dev requested a review from bluwy March 18, 2022 17:11
@patak-dev patak-dev added the p3-minor-bug An edge case that only affects very specific usage (priority) label Mar 18, 2022
@patak-dev
Copy link
Member Author

✅ All green vite-ecosystem-ci run against this PR
https://github.com/vitejs/vite-ecosystem-ci/actions/runs/2005480978

@bluwy
Copy link
Member

bluwy commented Mar 19, 2022

Some questions 😬

7. There are common chunks between them so dep_a ends up with bh_a but with bundle o_a2

I'm curious how/why dep_a ends up with bh_a, given IIUC the browserHash is calculated based on the deps found too (in this case a new dep_b which should create a new browserHash). Or maybe that isn't enough, I wonder if we should also calculate and derive via the imports too? (borrowed from alteredFiles)

b. It also fixes versioning of chunks and dynamic imports, as they should get a stable hash, and they can be safely cached long term using the main browserHash

I'm a bit confused of why we need another hash, given chunks are already hashed in their file name. I think I'm missing a piece here.

@patak-dev
Copy link
Member Author

Some questions 😬

  1. There are common chunks between them so dep_a ends up with bh_a but with bundle o_a2

I'm curious how/why dep_a ends up with bh_a, given IIUC the browserHash is calculated based on the deps found too (in this case a new dep_b which should create a new browserHash). Or maybe that isn't enough, I wonder if we should also calculate and derive via the imports too? (borrowed from alteredFiles)

Each discovered dep gets a new browser hash based on every optimized + discovered dep, so deb_b will get a new bh_b has. But dep_a will keep bh_a until we need to do a full-page reload (we can't change the hash of a dep of we will have duplicates). So dep_a hash didn't include dep_b.

b. It also fixes versioning of chunks and dynamic imports, as they should get a stable hash, and they can be safely cached long term using the main browserHash

I'm a bit confused of why we need another hash, given chunks are already hashed in their file name. I think I'm missing a piece here.

Import analysis is also performed in these chunks adding the browser hash to its imported paths, so the hash from the file name isn't enough because for the same file content we may end up needing to use different browser hashes in these imports. See also #7350, this was the problem in #7323

Copy link
Member

@bluwy bluwy left a comment

Choose a reason for hiding this comment

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

After re-reading the code a few times, I think I understand most of the whys here now. LGTM.

@patak-dev patak-dev merged commit 392a0de into main Mar 19, 2022
@patak-dev patak-dev deleted the fix/deps-long-term-caching branch March 19, 2022 07:47
@patak-dev
Copy link
Member Author

Released in vite@2.9.0-beta.4

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
p3-minor-bug An edge case that only affects very specific usage (priority)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Vite server crashes when starting dev server after optimizing new dependencies [ERR_OUTDATED_OPTIMIZED_DEP]
2 participants