Skip to content

Commit 5a28a29

Browse files
authored
fix(perf): lazy load workspace dependency (#7352)
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)
1 parent 5fc0f9d commit 5a28a29

File tree

3 files changed

+54
-42
lines changed

3 files changed

+54
-42
lines changed

lib/utils/log-file.js

+32-10
Original file line numberDiff line numberDiff line change
@@ -1,15 +1,13 @@
11
const os = require('os')
22
const { join, dirname, basename } = require('path')
33
const { format } = require('util')
4-
const { glob } = require('glob')
54
const { Minipass } = require('minipass')
65
const fsMiniPass = require('fs-minipass')
76
const fs = require('fs/promises')
87
const log = require('./log-shim')
98
const Display = require('./display')
109

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

1412
class LogFiles {
1513
// Default to a plain minipass stream so we can buffer
@@ -199,25 +197,49 @@ class LogFiles {
199197

200198
try {
201199
const logPath = this.#getLogFilePath()
202-
const logGlob = join(dirname(logPath), basename(logPath)
200+
const patternFileName = basename(logPath)
203201
// tell glob to only match digits
204-
.replace(/\d/g, '[0123456789]')
202+
.replace(/\d/g, 'd')
205203
// Handle the old (prior to 8.2.0) log file names which did not have a
206204
// counter suffix
207-
.replace(/-\.log$/, '*.log')
208-
)
205+
.replace('-.log', '')
206+
207+
let files = await fs.readdir(
208+
dirname(logPath), {
209+
withFileTypes: true,
210+
encoding: 'utf-8',
211+
})
212+
files = files.sort((a, b) => basename(a.name).localeCompare(basename(b.name), 'en'))
213+
214+
const logFiles = []
215+
216+
for (const file of files) {
217+
if (!file.isFile()) {
218+
continue
219+
}
220+
221+
const genericFileName = file.name.replace(/\d/g, 'd')
222+
const filePath = join(dirname(logPath), basename(file.name))
223+
224+
// Always ignore the currently written files
225+
if (
226+
genericFileName.includes(patternFileName)
227+
&& genericFileName.endsWith('.log')
228+
&& !this.#files.includes(filePath)
229+
) {
230+
logFiles.push(filePath)
231+
}
232+
}
209233

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

214236
if (toDelete <= 0) {
215237
return
216238
}
217239

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

220-
for (const file of files.slice(0, toDelete)) {
242+
for (const file of logFiles.slice(0, toDelete)) {
221243
try {
222244
await fs.rm(file, { force: true })
223245
} catch (e) {

test/lib/utils/log-file.js

+21-31
Original file line numberDiff line numberDiff line change
@@ -57,8 +57,10 @@ const loadLogFile = async (t, { buffer = [], mocks, testdir = {}, ...options } =
5757
logFile,
5858
LogFile,
5959
readLogs: async () => {
60-
const logDir = await fs.readdir(root)
61-
const logFiles = logDir.map((f) => path.join(root, f))
60+
const logDir = await fs.readdir(root, { withFileTypes: true })
61+
const logFiles = logDir
62+
.filter(f => f.isFile())
63+
.map((f) => path.join(root, f.name))
6264
.filter((f) => _fs.existsSync(f))
6365
return Promise.all(logFiles.map(async (f) => {
6466
const content = await fs.readFile(f, 'utf8')
@@ -202,6 +204,22 @@ t.test('cleans logs', async t => {
202204
t.equal(logs.length, logsMax + 1)
203205
})
204206

207+
t.test('cleans logs even when find folder inside logs folder', async t => {
208+
const logsMax = 5
209+
const { readLogs } = await loadLogFile(t, {
210+
logsMax,
211+
testdir: {
212+
...makeOldLogs(10),
213+
ignore_folder: {
214+
'ignored-file.txt': 'hello',
215+
},
216+
},
217+
})
218+
219+
const logs = await readLogs()
220+
t.equal(logs.length, logsMax + 1)
221+
})
222+
205223
t.test('doesnt clean current log by default', async t => {
206224
const logsMax = 1
207225
const { readLogs, logFile } = await loadLogFile(t, {
@@ -240,35 +258,6 @@ t.test('doesnt need to clean', async t => {
240258
t.equal(logs.length, oldLogs + 1)
241259
})
242260

243-
t.test('glob error', async t => {
244-
const { readLogs } = await loadLogFile(t, {
245-
logsMax: 5,
246-
mocks: {
247-
glob: { glob: () => {
248-
throw new Error('bad glob')
249-
} },
250-
},
251-
})
252-
253-
const logs = await readLogs()
254-
t.equal(logs.length, 1)
255-
t.match(last(logs).content, /error cleaning log files .* bad glob/)
256-
})
257-
258-
t.test('do not log cleaning errors when logging is disabled', async t => {
259-
const { readLogs } = await loadLogFile(t, {
260-
logsMax: 0,
261-
mocks: {
262-
glob: () => {
263-
throw new Error('should not be logged')
264-
},
265-
},
266-
})
267-
268-
const logs = await readLogs()
269-
t.equal(logs.length, 0)
270-
})
271-
272261
t.test('cleans old style logs too', async t => {
273262
const logsMax = 5
274263
const oldLogs = 10
@@ -290,6 +279,7 @@ t.test('rimraf error', async t => {
290279
testdir: makeOldLogs(oldLogs),
291280
mocks: {
292281
'fs/promises': {
282+
readdir: fs.readdir,
293283
rm: async (...args) => {
294284
if (count >= 3) {
295285
throw new Error('bad rimraf')

workspaces/config/lib/index.js

+1-1
Original file line numberDiff line numberDiff line change
@@ -2,7 +2,6 @@
22
const { walkUp } = require('walk-up-path')
33
const ini = require('ini')
44
const nopt = require('nopt')
5-
const mapWorkspaces = require('@npmcli/map-workspaces')
65
const rpj = require('read-package-json-fast')
76
const log = require('proc-log')
87

@@ -704,6 +703,7 @@ class Config {
704703
continue
705704
}
706705

706+
const mapWorkspaces = require('@npmcli/map-workspaces')
707707
const workspaces = await mapWorkspaces({ cwd: p, pkg })
708708
for (const w of workspaces.values()) {
709709
if (w === this.localPrefix) {

0 commit comments

Comments
 (0)