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(perf): lazy load workspace dependency #7352

Merged
merged 2 commits into from
Apr 9, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
42 changes: 32 additions & 10 deletions lib/utils/log-file.js
Original file line number Diff line number Diff line change
@@ -1,15 +1,13 @@
const os = require('os')
const { join, dirname, basename } = require('path')
const { format } = require('util')
const { glob } = require('glob')
const { Minipass } = require('minipass')
const fsMiniPass = require('fs-minipass')
const fs = require('fs/promises')
const log = require('./log-shim')
const Display = require('./display')

const padZero = (n, length) => n.toString().padStart(length.toString().length, '0')
const globify = pattern => pattern.split('\\').join('/')

class LogFiles {
// Default to a plain minipass stream so we can buffer
Expand Down Expand Up @@ -199,25 +197,49 @@ class LogFiles {

try {
const logPath = this.#getLogFilePath()
const logGlob = join(dirname(logPath), basename(logPath)
const patternFileName = basename(logPath)
// tell glob to only match digits
.replace(/\d/g, '[0123456789]')
.replace(/\d/g, 'd')
// Handle the old (prior to 8.2.0) log file names which did not have a
// counter suffix
.replace(/-\.log$/, '*.log')
)
.replace('-.log', '')

let files = await fs.readdir(
dirname(logPath), {
withFileTypes: true,
encoding: 'utf-8',
})
files = files.sort((a, b) => basename(a.name).localeCompare(basename(b.name), 'en'))

const logFiles = []

for (const file of files) {
if (!file.isFile()) {
continue
}

const genericFileName = file.name.replace(/\d/g, 'd')
const filePath = join(dirname(logPath), basename(file.name))

// Always ignore the currently written files
if (
genericFileName.includes(patternFileName)
&& genericFileName.endsWith('.log')
&& !this.#files.includes(filePath)
) {
logFiles.push(filePath)
}
}

// Always ignore the currently written files
const files = await glob(globify(logGlob), { ignore: this.#files.map(globify), silent: true })
const toDelete = files.length - this.#logsMax
const toDelete = logFiles.length - this.#logsMax

if (toDelete <= 0) {
return
}

log.silly('logfile', `start cleaning logs, removing ${toDelete} files`)

for (const file of files.slice(0, toDelete)) {
for (const file of logFiles.slice(0, toDelete)) {
try {
await fs.rm(file, { force: true })
} catch (e) {
Expand Down
52 changes: 21 additions & 31 deletions test/lib/utils/log-file.js
Original file line number Diff line number Diff line change
Expand Up @@ -57,8 +57,10 @@ const loadLogFile = async (t, { buffer = [], mocks, testdir = {}, ...options } =
logFile,
LogFile,
readLogs: async () => {
const logDir = await fs.readdir(root)
const logFiles = logDir.map((f) => path.join(root, f))
const logDir = await fs.readdir(root, { withFileTypes: true })
const logFiles = logDir
.filter(f => f.isFile())
.map((f) => path.join(root, f.name))
.filter((f) => _fs.existsSync(f))
return Promise.all(logFiles.map(async (f) => {
const content = await fs.readFile(f, 'utf8')
Expand Down Expand Up @@ -202,6 +204,22 @@ t.test('cleans logs', async t => {
t.equal(logs.length, logsMax + 1)
})

t.test('cleans logs even when find folder inside logs folder', async t => {
const logsMax = 5
const { readLogs } = await loadLogFile(t, {
logsMax,
testdir: {
...makeOldLogs(10),
ignore_folder: {
'ignored-file.txt': 'hello',
},
},
})

const logs = await readLogs()
t.equal(logs.length, logsMax + 1)
})

t.test('doesnt clean current log by default', async t => {
const logsMax = 1
const { readLogs, logFile } = await loadLogFile(t, {
Expand Down Expand Up @@ -240,35 +258,6 @@ t.test('doesnt need to clean', async t => {
t.equal(logs.length, oldLogs + 1)
})

t.test('glob error', async t => {
const { readLogs } = await loadLogFile(t, {
logsMax: 5,
mocks: {
glob: { glob: () => {
throw new Error('bad glob')
} },
},
})

const logs = await readLogs()
t.equal(logs.length, 1)
t.match(last(logs).content, /error cleaning log files .* bad glob/)
})

t.test('do not log cleaning errors when logging is disabled', async t => {
const { readLogs } = await loadLogFile(t, {
logsMax: 0,
mocks: {
glob: () => {
throw new Error('should not be logged')
},
},
})

const logs = await readLogs()
t.equal(logs.length, 0)
})

t.test('cleans old style logs too', async t => {
const logsMax = 5
const oldLogs = 10
Expand All @@ -290,6 +279,7 @@ t.test('rimraf error', async t => {
testdir: makeOldLogs(oldLogs),
mocks: {
'fs/promises': {
readdir: fs.readdir,
rm: async (...args) => {
if (count >= 3) {
throw new Error('bad rimraf')
Expand Down
2 changes: 1 addition & 1 deletion workspaces/config/lib/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,6 @@
const { walkUp } = require('walk-up-path')
const ini = require('ini')
const nopt = require('nopt')
const mapWorkspaces = require('@npmcli/map-workspaces')
const rpj = require('read-package-json-fast')
const log = require('proc-log')

Expand Down Expand Up @@ -704,6 +703,7 @@ class Config {
continue
}

const mapWorkspaces = require('@npmcli/map-workspaces')
wraithgar marked this conversation as resolved.
Show resolved Hide resolved
const workspaces = await mapWorkspaces({ cwd: p, pkg })
for (const w of workspaces.values()) {
if (w === this.localPrefix) {
Expand Down
Loading