small optimization

[sirenkovladd]

What

1. Reduced barrel issue
2. small optimization with process.title
3. moved the optional require

Describe the request in detail. What it does and why it's being changed.

1. Barrel issue is known for importing files that you don't really need
   it takes extra time when running any code (this reduced execution time by ~15%)

const { cleanUrl } = require('npm-registry-fetch')
// ->
const cleanUrl = require('npm-registry-fetch/lib/clean-url')

2. changing process.title is a synchronous operation that blocks the thread, it is better to reduce the number of calls of this command (allowed to reduce the execution time by 5%)

3. moved the optional require, imports only what is used, ./utils/replace-info.js is quite a heavy dependency, moving it to the function where it is called allowed to reduce the time by 5%

Result:

❯ hyperfine -w 5 -r 20 'npm run echo' 'node bin/npm-cli.js run echo'
Benchmark 1: npm run echo
  Time (mean ± σ):     256.4 ms ±   5.2 ms    [User: 253.4 ms, System: 34.6 ms]
  Range (min … max):   251.4 ms … 273.2 ms    20 runs
 
Benchmark 2: node bin/npm-cli.js run echo
  Time (mean ± σ):     201.5 ms ±   5.0 ms    [User: 183.7 ms, System: 24.1 ms]
  Range (min … max):   197.7 ms … 221.4 ms    20 runs
 
Summary
  node bin/npm-cli.js run echo ran
    1.27 ± 0.04 times faster than npm run echo

[wraithgar]

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.

[ljharb]

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

[sirenkovladd]

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?

[wraithgar]

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.

[sirenkovladd]

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

[wraithgar]

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.

[wraithgar]

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

[sirenkovladd]

❯ hyperfine -w 5 -r 20 'npm run echo' 'node bin/npm-cli.js run echo'
Benchmark 1: npm run echo
  Time (mean ± σ):     253.7 ms ±   4.0 ms    [User: 252.9 ms, System: 34.0 ms]
  Range (min … max):   250.2 ms … 268.7 ms    20 runs
 
Benchmark 2: node bin/npm-cli.js run echo
  Time (mean ± σ):     208.4 ms ±   5.0 ms    [User: 184.4 ms, System: 24.1 ms]
  Range (min … max):   205.9 ms … 229.5 ms    20 runs
 
  Warning: Statistical outliers were detected. Consider re-running this benchmark on a quiet system without any interferences from other programs. It might help to use the '--warmup' or '--prepare' options.
 
Summary
  node bin/npm-cli.js run echo ran
    1.22 ± 0.04 times faster than npm run echo

[wraithgar]

This has been addressed by #7334