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 loading optimizations #7388

Merged
merged 1 commit into from
Apr 18, 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
10 changes: 4 additions & 6 deletions lib/base-command.js
Original file line number Diff line number Diff line change
@@ -1,9 +1,3 @@
// Base class for npm commands

const { relative } = require('path')

const { definitions } = require('@npmcli/config/lib/definitions')
const { aliases: cmdAliases } = require('./utils/cmd-list')
const { log, output } = require('proc-log')

class BaseCommand {
Expand All @@ -18,6 +12,8 @@ class BaseCommand {
// this is a static so that we can read from it without instantiating a command
// which would require loading the config
static get describeUsage () {
const { definitions } = require('@npmcli/config/lib/definitions')
const { aliases: cmdAliases } = require('./utils/cmd-list')
const seenExclusive = new Set()
const wrapWidth = 80
const { description, usage = [''], name, params } = this
Expand Down Expand Up @@ -161,6 +157,8 @@ class BaseCommand {
}

async setWorkspaces () {
const { relative } = require('node:path')

const includeWorkspaceRoot = this.isArboristCmd
? false
: this.npm.config.get('include-workspace-root')
Expand Down
2 changes: 1 addition & 1 deletion lib/cli.js
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
const validateEngines = require('./es6/validate-engines.js')
const cliEntry = require('path').resolve(__dirname, 'cli-entry.js')
const cliEntry = require('node:path').resolve(__dirname, 'cli-entry.js')

module.exports = (process) => validateEngines(process, () => require(cliEntry))
10 changes: 5 additions & 5 deletions lib/commands/config.js
Original file line number Diff line number Diff line change
@@ -1,8 +1,7 @@
const { mkdir, readFile, writeFile } = require('fs/promises')
const { dirname, resolve } = require('path')
const { spawn } = require('child_process')
const { EOL } = require('os')
const ini = require('ini')
const { mkdir, readFile, writeFile } = require('node:fs/promises')
const { dirname, resolve } = require('node:path')
const { spawn } = require('node:child_process')
const { EOL } = require('node:os')
const localeCompare = require('@isaacs/string-locale-compare')('en')
const pkgJson = require('@npmcli/package-json')
const { defaults, definitions } = require('@npmcli/config/lib/definitions')
Expand Down Expand Up @@ -201,6 +200,7 @@ class Config extends BaseCommand {
}

async edit () {
const ini = require('ini')
const e = this.npm.flatOptions.editor
const where = this.npm.flatOptions.location
const file = this.npm.config.data.get(where).source
Expand Down
4 changes: 2 additions & 2 deletions lib/commands/help-search.js
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
const { readFile } = require('fs/promises')
const path = require('path')
const { readFile } = require('node:fs/promises')
const path = require('node:path')
const { glob } = require('glob')
const { output } = require('proc-log')
const BaseCommand = require('../base-command.js')
Expand Down
9 changes: 3 additions & 6 deletions lib/commands/install.js
Original file line number Diff line number Diff line change
@@ -1,13 +1,10 @@
/* eslint-disable camelcase */
const fs = require('fs')
const util = require('util')
const readdir = util.promisify(fs.readdir)
const reifyFinish = require('../utils/reify-finish.js')
const { readdir } = require('node:fs/promises')
const { resolve, join } = require('node:path')
const { log } = require('proc-log')
const { resolve, join } = require('path')
const runScript = require('@npmcli/run-script')
const pacote = require('pacote')
const checks = require('npm-install-checks')
const reifyFinish = require('../utils/reify-finish.js')

const ArboristWorkspaceCmd = require('../arborist-cmd.js')
class Install extends ArboristWorkspaceCmd {
Expand Down
2 changes: 1 addition & 1 deletion lib/commands/query.js
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
'use strict'

const { resolve } = require('path')
const { resolve } = require('node:path')
const BaseCommand = require('../base-command.js')
const { log, output } = require('proc-log')

Expand Down
40 changes: 19 additions & 21 deletions lib/commands/run-script.js
Original file line number Diff line number Diff line change
@@ -1,20 +1,5 @@
const runScript = require('@npmcli/run-script')
const { isServerPackage } = runScript
const pkgJson = require('@npmcli/package-json')
const { log, output } = require('proc-log')
const didYouMean = require('../utils/did-you-mean.js')
const { isWindowsShell } = require('../utils/is-windows.js')

const cmdList = [
'publish',
'install',
'uninstall',
'test',
'stop',
'start',
'restart',
'version',
].reduce((l, p) => l.concat(['pre' + p, p, 'post' + p]), [])
const pkgJson = require('@npmcli/package-json')

const BaseCommand = require('../base-command.js')
class RunScript extends BaseCommand {
Expand Down Expand Up @@ -64,9 +49,7 @@ class RunScript extends BaseCommand {
}

async run ([event, ...args], { path = this.npm.localPrefix, pkg } = {}) {
// this || undefined is because runScript will be unhappy with the default
// null value
const scriptShell = this.npm.config.get('script-shell') || undefined
const runScript = require('@npmcli/run-script')

if (!pkg) {
const { content } = await pkgJson.normalize(path)
Expand All @@ -77,19 +60,21 @@ class RunScript extends BaseCommand {
if (event === 'restart' && !scripts.restart) {
scripts.restart = 'npm stop --if-present && npm start'
} else if (event === 'env' && !scripts.env) {
const { isWindowsShell } = require('../utils/is-windows.js')
scripts.env = isWindowsShell ? 'SET' : 'env'
}

pkg.scripts = scripts

if (
!Object.prototype.hasOwnProperty.call(scripts, event) &&
!(event === 'start' && (await isServerPackage(path)))
!(event === 'start' && (await runScript.isServerPackage(path)))
) {
if (this.npm.config.get('if-present')) {
return
}

const didYouMean = require('../utils/did-you-mean.js')
const suggestions = await didYouMean(path, event)
throw new Error(
`Missing script: "${event}"${suggestions}\n\nTo see a list of scripts, run:\n npm run`
Expand All @@ -111,7 +96,9 @@ class RunScript extends BaseCommand {
for (const [ev, evArgs] of events) {
await runScript({
path,
scriptShell,
// this || undefined is because runScript will be unhappy with the
// default null value
scriptShell: this.npm.config.get('script-shell') || undefined,
stdio: 'inherit',
pkg,
event: ev,
Expand Down Expand Up @@ -147,6 +134,17 @@ class RunScript extends BaseCommand {
return allScripts
}

// TODO this is missing things like prepare, prepublishOnly, and dependencies
const cmdList = [
'preinstall', 'install', 'postinstall',
'prepublish', 'publish', 'postpublish',
'prerestart', 'restart', 'postrestart',
'prestart', 'start', 'poststart',
'prestop', 'stop', 'poststop',
'pretest', 'test', 'posttest',
'preuninstall', 'uninstall', 'postuninstall',
'preversion', 'version', 'postversion',
]
const indent = '\n '
const prefix = ' '
const cmds = []
Expand Down
41 changes: 18 additions & 23 deletions lib/commands/version.js
Original file line number Diff line number Diff line change
@@ -1,10 +1,7 @@
const libnpmversion = require('libnpmversion')
const { resolve } = require('path')
const { promisify } = require('util')
const { resolve } = require('node:path')
const { readFile } = require('node:fs/promises')
const { output } = require('proc-log')
const readFile = promisify(require('fs').readFile)

const updateWorkspaces = require('../workspaces/update-workspaces.js')
const BaseCommand = require('../base-command.js')

class Version extends BaseCommand {
Expand Down Expand Up @@ -74,6 +71,7 @@ class Version extends BaseCommand {
}

async change (args) {
const libnpmversion = require('libnpmversion')
const prefix = this.npm.config.get('tag-version-prefix')
const version = await libnpmversion(args[0], {
...this.npm.flatOptions,
Expand All @@ -83,20 +81,33 @@ class Version extends BaseCommand {
}

async changeWorkspaces (args) {
const updateWorkspaces = require('../workspaces/update-workspaces.js')
const libnpmversion = require('libnpmversion')
const prefix = this.npm.config.get('tag-version-prefix')
const {
config,
flatOptions,
localPrefix,
} = this.npm
await this.setWorkspaces()
const updatedWorkspaces = []
for (const [name, path] of this.workspaces) {
output.standard(name)
const version = await libnpmversion(args[0], {
...this.npm.flatOptions,
...flatOptions,
'git-tag-version': false,
path,
})
updatedWorkspaces.push(name)
output.standard(`${prefix}${version}`)
}
return this.update(updatedWorkspaces)
return updateWorkspaces({
config,
flatOptions,
localPrefix,
npm: this.npm,
workspaces: updatedWorkspaces,
})
}

async list (results = {}) {
Expand Down Expand Up @@ -136,22 +147,6 @@ class Version extends BaseCommand {
}
return this.list(results)
}

async update (workspaces) {
const {
config,
flatOptions,
localPrefix,
} = this.npm

await updateWorkspaces({
config,
flatOptions,
localPrefix,
npm: this.npm,
workspaces,
})
}
}

module.exports = Version
4 changes: 2 additions & 2 deletions lib/npm.js
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
const { resolve, dirname, join } = require('path')
const { resolve, dirname, join } = require('node:path')
const Config = require('@npmcli/config')
const which = require('which')
const fs = require('fs/promises')
const fs = require('node:fs/promises')

// Patch the global fs module here at the app level
require('graceful-fs').gracefulify(require('fs'))
Expand Down
2 changes: 1 addition & 1 deletion lib/package-url-cmd.js
Original file line number Diff line number Diff line change
@@ -1,7 +1,6 @@
// Base command for opening urls from a package manifest (bugs, docs, repo)

const pacote = require('pacote')
const hostedGitInfo = require('hosted-git-info')

const openUrl = require('./utils/open-url.js')
const { log } = require('proc-log')
Expand Down Expand Up @@ -52,6 +51,7 @@ class PackageUrlCommand extends BaseCommand {
// repository (if a string) or repository.url (if an object) returns null
// if it's not a valid repo, or not a known hosted repo
hostedFromMani (mani) {
const hostedGitInfo = require('hosted-git-info')
const r = mani.repository
const rurl = !r ? null
: typeof r === 'string' ? r
Expand Down
12 changes: 5 additions & 7 deletions lib/utils/error-message.js
Original file line number Diff line number Diff line change
@@ -1,7 +1,6 @@
const { format } = require('util')
const { resolve } = require('path')
const { format } = require('node:util')
const { resolve } = require('node:path')
const { redactLog: replaceInfo } = require('@npmcli/redact')
const { report } = require('./explain-eresolve.js')
const { log } = require('proc-log')

const messageText = msg => msg.map(line => line.slice(1).join(' ')).join('\n')
Expand Down Expand Up @@ -32,6 +31,7 @@ const errorMessage = (er, npm) => {

switch (er.code) {
case 'ERESOLVE': {
const { report } = require('./explain-eresolve.js')
short.push(['ERESOLVE', er.message])
detail.push(['', ''])
// XXX(display): error messages are logged so we use the logColor since that is based
Expand Down Expand Up @@ -77,9 +77,7 @@ const errorMessage = (er, npm) => {
npm.config.loaded &&
er.dest.startsWith(npm.config.get('cache'))

const { isWindows } = require('./is-windows.js')

if (!isWindows && (isCachePath || isCacheDest)) {
if (process.platform !== 'win32' && (isCachePath || isCacheDest)) {
// user probably doesn't need this, but still add it to the debug log
log.verbose(er.stack)
short.push([
Expand All @@ -101,7 +99,7 @@ const errorMessage = (er, npm) => {
'',
[
'\nThe operation was rejected by your operating system.',
isWindows
process.platform === 'win32'
/* eslint-disable-next-line max-len */
? "It's possible that the file was already in use (by a text editor or antivirus),\n" +
'or that you lack permissions to access it.'
Expand Down
4 changes: 2 additions & 2 deletions lib/utils/exit-handler.js
Original file line number Diff line number Diff line change
@@ -1,5 +1,3 @@
const os = require('os')
const fs = require('fs')
const { log, output, time } = require('proc-log')
const errorMessage = require('./error-message.js')
const { redactLog: replaceInfo } = require('@npmcli/redact')
Expand Down Expand Up @@ -149,6 +147,8 @@ const exitHandler = err => {
log.error('weird error', err)
noLogMessage = true
} else {
const os = require('node:os')
const fs = require('node:fs')
if (!err.code) {
const matchErrorCode = err.message.match(/^(?:Error: )?(E[A-Z]+)/)
err.code = matchErrorCode && matchErrorCode[1]
Expand Down
2 changes: 1 addition & 1 deletion lib/utils/explain-dep.js
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
const { relative } = require('path')
const { relative } = require('node:path')

const explainNode = (node, depth, chalk) =>
printNode(node, chalk) +
Expand Down
4 changes: 1 addition & 3 deletions lib/utils/is-windows.js
Original file line number Diff line number Diff line change
@@ -1,6 +1,4 @@
const isWindows = process.platform === 'win32'
const isWindowsShell = isWindows &&
const isWindowsShell = (process.platform === 'win32') &&
!/^MINGW(32|64)$/.test(process.env.MSYSTEM) && process.env.TERM !== 'cygwin'

exports.isWindows = isWindows
exports.isWindowsShell = isWindowsShell
4 changes: 2 additions & 2 deletions lib/utils/timers.js
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
const EE = require('events')
const fs = require('fs')
const EE = require('node:events')
const fs = require('node:fs')
const { log, time } = require('proc-log')

const INITIAL_TIMER = 'npm'
Expand Down
12 changes: 6 additions & 6 deletions test/lib/utils/exit-handler.js
Original file line number Diff line number Diff line change
@@ -1,8 +1,8 @@
const fs = require('node:fs')
const { join, resolve } = require('node:path')
const EventEmitter = require('node:events')
const t = require('tap')
const fs = require('fs')
const fsMiniPass = require('fs-minipass')
const { join, resolve } = require('path')
const EventEmitter = require('events')
const { output, time } = require('proc-log')
const { load: loadMockNpm } = require('../../fixtures/mock-npm')
const mockGlobals = require('@npmcli/mock-globals')
Expand Down Expand Up @@ -85,7 +85,7 @@ const mockExitHandler = async (t, { config, mocks, files, ...opts } = {}) => {
},
},
}),
os: {
'node:os': {
type: () => 'Foo',
release: () => '1.0.0',
},
Expand Down Expand Up @@ -367,7 +367,7 @@ t.test('no logs dir', async (t) => {
t.test('timers fail to write', async (t) => {
// we want the fs.writeFileSync in the Timers class to fail
const mockTimers = tmock(t, '{LIB}/utils/timers.js', {
fs: {
'node:fs': {
...fs,
writeFileSync: (file, ...rest) => {
if (file.includes('LOGS_DIR')) {
Expand Down Expand Up @@ -450,7 +450,7 @@ t.test('files from error message with error', async (t) => {
['error-file.txt', '# error file content'],
],
mocks: {
fs: {
'node:fs': {
...fs,
writeFileSync: (dir) => {
if (dir.includes('LOGS_DIR') && dir.endsWith('error-file.txt')) {
Expand Down
Loading