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.

Sign up for GitHub

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

Jump to bottom

fix: allow cache overlap in parallel builds #8592

Merged
merged 1 commit into from Jun 16, 2022
Merged

fix: allow cache overlap in parallel builds #8592

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
6 changes: 5 additions & 1 deletion packages/vite/src/node/optimizer/index.ts
View file
Open in desktop
Expand Up @@ -620,7 +620,11 @@ export function getOptimizedDepPath(
function getDepsCacheSuffix(config: ResolvedConfig): string {
let suffix = ''
if (config.command === 'build') {
suffix += '_build'
// Differentiate build caches depending on outDir to allow parallel builds
const { outDir } = config.build
const buildId =
outDir.length > 8 || outDir.includes('/') ? getHash(outDir) : outDir
Comment on lines +625 to +626
Copy link
Member

Choose a reason for hiding this comment

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

Maybe we can use flattenId here so that we don't have to potentially hash it? 🤔

Copy link
Member

Choose a reason for hiding this comment

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

good point. This makes the path easier to debug 👍

Copy link
Member Author

Choose a reason for hiding this comment

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

That is an interesting idea. So we could flatten and check that the length isn't too much? But maybe a limit like 16 🤔
My main issue was if there is a chance for the user to set an absolute or relative path out of root. But we could detect that, maybe it isn't even a thing?

Copy link
Member Author

Choose a reason for hiding this comment

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

There is a problem with using flattenId. We can safely use it with node deps because we know that _ isn't a valid char there. But here if we use it we could collide dist/foo with dist_foo. I agree it would be extremely weird to have both, but I don't feel that comfortable with making this a special case. I think we should leave the code as it is now. What do you think @bluwy?

Copy link
Member

Choose a reason for hiding this comment

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

That's true, I guess if we still want to, we can pre-replace _ with ____. My main concern is that getDepsCacheSuffix is a hot-code path, and potentially calling getHash many times may not be good for perf. Unless we refactor the optimizer to pre-calculate the cacheDir.

We could probably still use getHash if we plan to optimize it later on.

Copy link
Member Author

Choose a reason for hiding this comment

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

Ok, let's merge this now as it is an important fix and leave the optimization for a future PR. Right now the big majority of users won't hit getHash anyways.

suffix += `_build-${buildId}`
if (config.build.ssr) {
suffix += '_ssr'
}
Expand Down
3 changes: 1 addition & 2 deletions playground/assets/vite.config-relative-base.js
View file
Open in desktop
Expand Up @@ -22,6 +22,5 @@ module.exports = {
},
testConfig: {
baseRoute: '/relative-base/'
},
cacheDir: 'node_modules/.vite/relative-base'
}
}
3 changes: 1 addition & 2 deletions playground/assets/vite.config.js
View file
Open in desktop
Expand Up @@ -17,6 +17,5 @@ module.exports = {
assetsInlineLimit: 8192, // 8kb
manifest: true,
watch: {}
},
cacheDir: 'node_modules/.vite/foo'
}
}
3 changes: 1 addition & 2 deletions playground/worker/vite.config-es.js
View file
Open in desktop
Expand Up @@ -38,6 +38,5 @@ module.exports = vite.defineConfig({
}
}
}
],
cacheDir: 'node_modules/.vite/es'
]
})
3 changes: 1 addition & 2 deletions playground/worker/vite.config-relative-base.js
View file
Open in desktop
Expand Up @@ -40,6 +40,5 @@ module.exports = vite.defineConfig({
}
}
}
],
cacheDir: 'node_modules/.vite/relative-base'
]
})
3 changes: 0 additions & 3 deletions playground/worker/vite.config-sourcemap.js
View file
Open in desktop
Expand Up @@ -21,9 +21,6 @@ module.exports = vite.defineConfig((sourcemap) => {
}
}
},
cacheDir: `node_modules/.vite/iife-${
typeof sourcemap === 'boolean' ? 'sourcemap' : 'sourcemap-' + sourcemap
}`,
build: {
outDir: `dist/iife-${
typeof sourcemap === 'boolean' ? 'sourcemap' : 'sourcemap-' + sourcemap
Expand Down
3 changes: 1 addition & 2 deletions playground/worker/vite.config.js
View file
Open in desktop
Expand Up @@ -23,6 +23,5 @@ module.exports = vite.defineConfig({
entryFileNames: 'assets/[name].js'
}
}
},
cacheDir: 'node_modules/.vite/iife'
}
})