Skip to content

small optimization #7176

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

Closed
sirenkovladd wants to merge 2 commits into from
Closed

small optimization #7176

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
6977f89
small optimization
sirenkovladd Jan 24, 2024
f5d35fe
return title
sirenkovladd Jan 24, 2024
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
3 changes: 1 addition & 2 deletions lib/cli-entry.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,6 @@
module.exports = async (process, validateEngines) => {
// set it here so that regardless of what happens later, we don't
// leak any private CLI configs to other programs
process.title = 'npm'

// if npm is called as "npmg" or "npm_g", then run in global mode.
if (process.argv[1][process.argv[1].length - 1] === 'g') {
Expand Down Expand Up @@ -57,7 +56,7 @@ module.exports = async (process, validateEngines) => {
process.exitCode = 1
return exitHandler()
}

setTimeout(() => process.title = npm.title)
await npm.exec(cmd)
return exitHandler()
} catch (err) {
Expand Down
3 changes: 1 addition & 2 deletions lib/npm.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,6 @@ const LogFile = require('./utils/log-file.js')
const Timers = require('./utils/timers.js')
const Display = require('./utils/display.js')
const log = require('./utils/log-shim')
const replaceInfo = require('./utils/replace-info.js')
const updateNotifier = require('./utils/update-notifier.js')
const pkg = require('../package.json')
const { deref } = require('./utils/cmd-list.js')
Expand Down Expand Up @@ -177,7 +176,6 @@ class Npm {
}

set title (t) {
process.title = t
this.#title = t
}

Expand Down Expand Up @@ -227,6 +225,7 @@ class Npm {
// note: this MUST be shorter than the actual argv length, because it
// uses the same memory, so node will truncate it if it's too long.
this.time('npm:load:setTitle', () => {
const replaceInfo = require('./utils/replace-info.js')
const { parsedArgv: { cooked, remain } } = this.config
this.argv = remain
// Secrets are mostly in configs, so title is set using only the positional args
Expand Down
2 changes: 1 addition & 1 deletion lib/utils/replace-info.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
const { cleanUrl } = require('npm-registry-fetch')
const cleanUrl = require('npm-registry-fetch/lib/clean-url')
Copy link
Member

@wraithgar wraithgar Jan 24, 2024

Choose a reason for hiding this comment

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

We tend not to do this as it unnecessarily couples to the layout of the code, and not its exports. module.exports is the module, as far a our semver conventions are concerned. This allows us to completely refactor or rewrite a module and not make it a breaking change, as long as the exports are the same.

Copy link
Contributor

@ljharb ljharb Jan 24, 2024

Choose a reason for hiding this comment

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

The layout of internal files is already part of the api and subject to semver, unless the exports field is used to encapsulate them.

Copy link
Author

@sirenkovladd sirenkovladd Jan 24, 2024

Choose a reason for hiding this comment

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

so that the future refactor does not introduce destructive changes
I suggest adding an export field to the package.json of this module, will this solve this problem?

Copy link
Member

@wraithgar wraithgar Jan 24, 2024

Choose a reason for hiding this comment

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

Something like that would have to be a larger decision/discussion than just one repo, it falls under how we build all of our software. Currently we define a main entry .e.g. https://github.com/npm/npm-registry-fetch/blob/e58e8bc6c5b25d53a764f58190fc3a5c764a2e78/package.json#L5, and rely on module.exports to define the api.

exports is an alternative to main, not a replacement. We are not moving to exports right now so my original, single-line suggestion is all we really want to do right now.

The line as it is now was done very intentionally, and adding that function to module.exports was the way in which it was exported.

Copy link
Author

@sirenkovladd sirenkovladd Jan 24, 2024

Choose a reason for hiding this comment

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

To clarify, you suggest leaving require('npm-registry-fetch') even though it adds ~15% to the runtime

If the only problem is that you are just starting to migrate to declaring exports in package.json, then I could first send a PR to npm-registry-fetch so that I can painlessly import only cleanUrl

Copy link
Member

@wraithgar wraithgar Jan 24, 2024

Choose a reason for hiding this comment

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

Yes leave it. A pr to npm-registry-fetch would be rejected as we would want to centrally manage that entry in package.json from @npmcli/template-oss and enforce it on ALL our packages.

Copy link
Member

@wraithgar wraithgar Jan 24, 2024

Choose a reason for hiding this comment

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

38 milliseconds (15% of 256) for npm run is not something we need to hyper optimize. This is not a hot path like reification.

const isString = (v) => typeof v === 'string'

// split on \s|= similar to how nopt parses options
Expand Down