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

feat: log re-optimization reasons #15339

Merged
merged 6 commits into from
Dec 13, 2023
Merged

Conversation

antfu
Copy link
Member

@antfu antfu commented Dec 13, 2023

Description

So digging into nuxt/nuxt#24196 (comment) we found out the root cause is that the hash is inconsistent across multiple runs (while I need to patch Vite locally to see what has changed). While in a complex setup, some many plugins might cause this race condition and hard to identify. I think it would be better to let users be aware of when and why the optimization cache gets invalidated, so it would help plugin authors to make the more stable behaviors.

Additional context


What is the purpose of this pull request?

  • Bug fix
  • New Feature
  • Documentation update
  • Other

Before submitting the PR, please make sure you do the following

  • Read the Contributing Guidelines, especially the Pull Request Guidelines.
  • Check that there isn't already a PR that solves the problem the same way to avoid creating a duplicate.
  • Provide a description in this PR that addresses what the PR is solving, or reference the issue that it solves (e.g. fixes #123).
  • Update the corresponding documentation if needed.
  • Ideally, include relevant tests that fail without this PR but pass with it.

Copy link

stackblitz bot commented Dec 13, 2023

Review PR in StackBlitz Codeflow Run & review this pull request in StackBlitz Codeflow.

@patak-dev patak-dev changed the title feat: log re-optimization reaons feat: log re-optimization resaons Dec 13, 2023
const hash = getDepHash(config, ssr)
const lockfileHash = getLockfileHash(config, ssr)
const configHash = getConfigHash(config, ssr)
const hash = lockfileHash + configHash
Copy link
Member

Choose a reason for hiding this comment

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

We are missing getHash here

Suggested change
const hash = lockfileHash + configHash
const hash = getHash(lockfileHash + configHash)

Maybe it is better to have a composeHashes util so it is in sync with getDepHash

Copy link
Member Author

Choose a reason for hiding this comment

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

Oh I wasn't mean to do another getHash here... because they are hashes already. Do we care about the length getting doubled, or could we simplify contacting them? I'd fix getDepHash if so

Copy link
Member

Choose a reason for hiding this comment

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

The hash ends up used in the ?v in the optimized module id, good for it to be a hash

@patak-dev
Copy link
Member

This will be helpful. I was thinking we should merge it in 5.1 so we dont trigger cold starts in a patch but I just realized that the lock will change. So the change I asked on the other PR was not justified.

@antfu
Copy link
Member Author

antfu commented Dec 13, 2023

Yeah, lock will change anyway. So we don't need to care about cross-version cache compatibility. I was thought you mean that include: undefined and include: [] will have different behavior

@patak-dev patak-dev changed the title feat: log re-optimization resaons feat: log re-optimization reasons Dec 13, 2023
Comment on lines +379 to +387
if (cachedMetadata.lockfileHash !== getLockfileHash(config, ssr)) {
config.logger.info(
'Re-optimizing dependencies because lockfile has changed',
)
} else if (cachedMetadata.configHash !== getConfigHash(config, ssr)) {
config.logger.info(
'Re-optimizing dependencies because vite config has changed',
)
} else {
Copy link
Member

Choose a reason for hiding this comment

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

Just a note, we don't need to account for the transition, when cachedMetadata.lockfileHash and cachedMetadata.configHash are undefined. In that case, the log will be that the lockfile has changed (because of the Vite update) so we are good

@patak-dev patak-dev merged commit b1a6c84 into main Dec 13, 2023
16 checks passed
@patak-dev patak-dev deleted the feat/log-re-opimization-reasons branch December 13, 2023 14:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants