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: avoid temporal optimize deps dirs #12582

Merged
merged 5 commits into from Mar 26, 2023
Merged

fix: avoid temporal optimize deps dirs #12582

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
3f3024c
fix: avoid temporal optimize deps dirs
patak-dev Mar 25, 2023
3c69081
chore: windows
patak-dev Mar 25, 2023
7a3e2b1
chore: update
patak-dev Mar 26, 2023
6b12faf
chore: update
patak-dev Mar 26, 2023
5ca1464
chore: simplify
patak-dev Mar 26, 2023
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
170 changes: 70 additions & 100 deletions packages/vite/src/node/optimizer/index.ts
View file
Open in desktop
Expand Up @@ -20,11 +20,8 @@ import {
lookupFile,
normalizeId,
normalizePath,
removeDir,
removeLeadingSlash,
renameDir,
tryStatSync,
writeFile,
} from '../utils'
import { transformWithEsbuild } from '../plugins/esbuild'
import { ESBUILD_MODULES_TARGET } from '../constants'
Expand Down Expand Up @@ -164,6 +161,9 @@ export interface DepOptimizationResult {
* to be able to discard the result
*/
commit: () => Promise<void>
/**
* @deprecated noop
*/
cancel: () => void
}

Expand Down Expand Up @@ -474,23 +474,6 @@ export function runOptimizeDeps(
}

const depsCacheDir = getDepsCacheDir(resolvedConfig, ssr)
const processingCacheDir = getProcessingDepsCacheDir(resolvedConfig, ssr)

// Create a temporal directory so we don't need to delete optimized deps
// until they have been processed. This also avoids leaving the deps cache
// directory in a corrupted state if there is an error
if (fs.existsSync(processingCacheDir)) {
emptyDir(processingCacheDir)
} else {
fs.mkdirSync(processingCacheDir, { recursive: true })
}

// a hint for Node.js
// all files in the cache directory should be recognized as ES modules
writeFile(
path.resolve(processingCacheDir, 'package.json'),
JSON.stringify({ type: 'module' }),
)

const metadata = initDepsOptimizerMetadata(config, ssr)

Expand All @@ -505,38 +488,16 @@ export function runOptimizeDeps(

const qualifiedIds = Object.keys(depsInfo)

let cleaned = false
const cleanUp = () => {
if (!cleaned) {
cleaned = true
// No need to wait, we can clean up in the background because temp folders
// are unique per run
fsp.rm(processingCacheDir, { recursive: true, force: true }).catch(() => {
// Ignore errors
})
}
}
const createProcessingResult = () => ({
const createEmptyProcessingResult = () => ({
metadata,
async commit() {
if (cleaned) {
throw new Error(
`Vite Internal Error: Can't commit optimizeDeps processing result, it has already been cancelled.`,
)
}
// Write metadata file, delete `deps` folder and rename the `processing` folder to `deps`
// Processing is done, we can now replace the depsCacheDir with processingCacheDir
// Rewire the file paths from the temporal processing dir to the final deps cache dir
await removeDir(depsCacheDir)
await renameDir(processingCacheDir, depsCacheDir)
},
cancel: cleanUp,
commit: async () => {},
cancel: async () => {},
})

if (!qualifiedIds.length) {
return {
cancel: async () => cleanUp(),
result: Promise.resolve(createProcessingResult()),
result: Promise.resolve(createEmptyProcessingResult()),
cancel: async () => {},
}
}

Expand All @@ -546,19 +507,19 @@ export function runOptimizeDeps(
resolvedConfig,
depsInfo,
ssr,
processingCacheDir,
depsCacheDir,
optimizerContext,
)

const result = preparedRun.then(({ context, idToExports }) => {
const runResult = preparedRun.then(({ context, idToExports }) => {
function disposeContext() {
return context?.dispose().catch((e) => {
config.logger.error('Failed to dispose esbuild context', { error: e })
})
}
if (!context || optimizerContext.cancelled) {
disposeContext()
return createProcessingResult()
return createEmptyProcessingResult()
}

return context
Expand All @@ -569,15 +530,11 @@ export function runOptimizeDeps(
// the paths in `meta.outputs` are relative to `process.cwd()`
const processingCacheDirOutputPath = path.relative(
process.cwd(),
processingCacheDir,
depsCacheDir,
)

for (const id in depsInfo) {
const output = esbuildOutputFromId(
meta.outputs,
id,
processingCacheDir,
)
const output = esbuildOutputFromId(meta.outputs, id, depsCacheDir)

const { exportsData, ...info } = depsInfo[id]
addOptimizedDepInfo(metadata, 'optimized', {
Expand Down Expand Up @@ -624,23 +581,58 @@ export function runOptimizeDeps(
}
}

const dataPath = path.join(processingCacheDir, '_metadata.json')
writeFile(
dataPath,
stringifyDepsOptimizerMetadata(metadata, depsCacheDir),
)

debug(
`Dependencies bundled in ${(performance.now() - start).toFixed(2)}ms`,
)

return createProcessingResult()
return {
metadata,
async commit() {
// Write metadata file, delete `deps` folder and rename the `processing` folder to `deps`
// Processing is done, we can now replace the depsCacheDir with processingCacheDir
// Rewire the file paths from the temporal processing dir to the final deps cache dir
patak-dev marked this conversation as resolved.
Show resolved Hide resolved

if (!fs.existsSync(depsCacheDir)) {
fs.mkdirSync(depsCacheDir, { recursive: true })
}

const files = []

// a hint for Node.js
// all files in the cache directory should be recognized as ES modules
files.push(
fsp.writeFile(
path.resolve(depsCacheDir, 'package.json'),
JSON.stringify({ type: 'module' }),
Copy link
Member

@sun0day sun0day Mar 26, 2023 •
edited

Choose a reason for hiding this comment

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

Would there be duplicated calls of JSON.stringify?

Copy link
Member

Choose a reason for hiding this comment

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

I feel like doing a writeFile instead of checking existence before writing would be a bit faster, though perf doesn't matter a lot I think since these optimized files are only served to the browser.

Copy link
Member Author

Choose a reason for hiding this comment

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

@sun0day changed it to be directly a string so we can have the same formatting as other files in the deps cache. We could use JSON.stringify({ type: 'module' }, null, 2) + '\n' too but the direct string feels better here.

@bluwy I added the check because I think the normal flow would be that this file is already there.

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, I took that back after the latest changes as it was making the code more complex and I don't think it would make a diff

),
)

const dataPath = path.join(depsCacheDir, '_metadata.json')
files.push(
fsp.writeFile(
dataPath,
stringifyDepsOptimizerMetadata(metadata, depsCacheDir),
),
)

for (const outputFile of result.outputFiles!) {
files.push(fsp.writeFile(outputFile.path, outputFile.text))
}

await Promise.all(files)

// Clean up old files in the background
cleanupDepsCacheStaleFiles(depsCacheDir, metadata)
bluwy marked this conversation as resolved.
Show resolved Hide resolved
},
cancel: () => {},
}
})

.catch((e) => {
if (e.errors && e.message.includes('The build was canceled')) {
// esbuild logs an error when cancelling, but this is expected so
// return an empty result instead
return createProcessingResult()
return createEmptyProcessingResult()
}
throw e
})
Expand All @@ -649,18 +641,13 @@ export function runOptimizeDeps(
})
})

result.catch(() => {
cleanUp()
})

return {
async cancel() {
optimizerContext.cancelled = true
const { context } = await preparedRun
await context?.cancel()
cleanUp()
},
result,
result: runResult,
}
}

Expand Down Expand Up @@ -760,6 +747,9 @@ async function prepareEsbuildOptimizerRun(
absWorkingDir: process.cwd(),
entryPoints: Object.keys(flatIdDeps),
bundle: true,
// Don't write to disk, we'll only write the files if the build isn't invalidated
// by newly discovered dependencies
write: false,
// We can't use platform 'neutral', as esbuild has custom handling
// when the platform is 'node' or 'browser' that can't be emulated
// by using mainFields and conditions
Expand Down Expand Up @@ -934,15 +924,6 @@ export function getDepsCacheDir(config: ResolvedConfig, ssr: boolean): string {
return getDepsCacheDirPrefix(config) + getDepsCacheSuffix(config, ssr)
}

function getProcessingDepsCacheDir(config: ResolvedConfig, ssr: boolean) {
return (
getDepsCacheDirPrefix(config) +
getDepsCacheSuffix(config, ssr) +
'_temp_' +
getHash(Date.now().toString())
)
}

export function getDepsCacheDirPrefix(config: ResolvedConfig): string {
return normalizePath(path.resolve(config.cacheDir, 'deps'))
}
Expand Down Expand Up @@ -1306,28 +1287,17 @@ export async function optimizedDepNeedsInterop(
return depInfo?.needsInterop
}

const MAX_TEMP_DIR_AGE_MS = 24 * 60 * 60 * 1000
export async function cleanupDepsCacheStaleDirs(
config: ResolvedConfig,
): Promise<void> {
try {
const cacheDir = path.resolve(config.cacheDir)
if (fs.existsSync(cacheDir)) {
const dirents = await fsp.readdir(cacheDir, { withFileTypes: true })
for (const dirent of dirents) {
if (dirent.isDirectory() && dirent.name.includes('_temp_')) {
const tempDirPath = path.resolve(config.cacheDir, dirent.name)
const stats = await fsp.stat(tempDirPath).catch((_) => null)
if (
stats?.mtime &&
Date.now() - stats.mtime.getTime() > MAX_TEMP_DIR_AGE_MS
) {
await removeDir(tempDirPath)
}
}
async function cleanupDepsCacheStaleFiles(
depsCacheDir: string,
metadata: DepOptimizationMetadata,
) {
const files = await fsp.readdir(depsCacheDir)
for (const file of files) {
if (file !== 'package.json' && file !== '_metadata.json') {
const filePath = path.join(depsCacheDir, file)
if (!optimizedDepInfoFromFile(metadata, normalizePath(filePath))) {
fsp.unlink(filePath)
}
}
} catch (err) {
config.logger.error(err)
}
}
5 changes: 0 additions & 5 deletions packages/vite/src/node/server/index.ts
View file
Open in desktop
Expand Up @@ -35,7 +35,6 @@ import { cjsSsrResolveExternals } from '../ssr/ssrExternal'
import { ssrFixStacktrace, ssrRewriteStacktrace } from '../ssr/ssrStacktrace'
import { ssrTransform } from '../ssr/ssrTransform'
import {
cleanupDepsCacheStaleDirs,
getDepsOptimizer,
initDepsOptimizer,
initDevSsrDepsOptimizer,
Expand Down Expand Up @@ -693,10 +692,6 @@ export async function createServer(
await initServer()
}

// Fire a clean up of stale cache dirs, in case old processes didn't
// terminate correctly. Don't await this promise
cleanupDepsCacheStaleDirs(config)

return server
}

Expand Down