Skip to content

Commit

Permalink
fix(perf): lazy load workspace dependency (npm#7352)
Browse files Browse the repository at this point in the history
Lazy loading `workspaces` and `glob` dependencies, will avoid loading
the dependency when running inside a repo that is not a workspace and
also will not load `glob` when is not needed.

Before:


![image](https://github.com/npm/cli/assets/12551007/5d8d50f3-d855-4d00-964d-b6b0526361b0)

After:


![image](https://github.com/npm/cli/assets/12551007/75744909-0fcb-43cc-9986-ff68bc46a078)
  • Loading branch information
H4ad authored Apr 9, 2024
1 parent 5fc0f9d commit 5a28a29
Show file tree
Hide file tree
Showing 3 changed files with 54 additions and 42 deletions.
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')
const workspaces = await mapWorkspaces({ cwd: p, pkg })
for (const w of workspaces.values()) {
if (w === this.localPrefix) {
Expand Down

0 comments on commit 5a28a29

Please sign in to comment.