refactor: use core Node.js resolve function if possible

[timfish]

This is a copy of #25 which looks like it was opened before CI tests were added.

I've fixed the original PR to pass on all tested versions.

If we made the minimum supported Node version >= v8.10.0 | >= v9.3.0 then we could remove resolve as a dependency entirely but this would be a breaking change!

[cla-checker-service]

💚 CLA has been signed

[trentm]

Thanks, Tim!

Thoughts on using the faster isCore2b? I am fine with a "meh" on grounds that it isn't significant.

[trentm]

This <array>.includes(moduleName) is slower: from about 4-5 ns on my laptop, up to about 80ns per call to isCore.

benchmarks

Using this bench-isCore.mjs:

// https://github.com/evanwashere/mitata?tab=readme-ov-file#readme
import { run, bench, boxplot, summary } from 'mitata';
import * as Module from 'node:module';

const isCore1 = Module.isBuiltin

const isCore2 = moduleName => {
  if (moduleName.startsWith('node:')) {
    return true
  }
  return Module.builtinModules.includes(moduleName)
}

const builtinModulesMap = {}
Module.builtinModules.forEach(k => { builtinModulesMap[k] = true; })
const isCore2b = moduleName => {
  if (moduleName.startsWith('node:')) {
    return true
  }
  return !!builtinModulesMap[moduleName]
}

import * as resolve from 'resolve';
const resolveCore = resolve.default.core;
const isCore3 = moduleName => {
  // Prefer `resolve.core` lookup to `resolve.isCore(moduleName)` because
  // the latter is doing version range matches for every call.
  return !!resolveCore[moduleName]
}

summary(() => {
  bench('isCore1("foo")', () => isCore1("foo"))
  bench('isCore2("foo")', () => isCore2("foo"))
  bench('isCore2b("foo")', () => isCore2b("foo"))
  bench('isCore3("foo")', () => isCore3("foo"))
})

await run();

Running that on my laptop gives:

% node bench-isCore.mjs
clk: ~3.85 GHz
cpu: Apple M3 Pro
runtime: node 18.20.4 (arm64-darwin)

benchmark                   avg (min … max) p75 / p99    (min … top 1%)
------------------------------------------- -------------------------------
isCore1("foo")                 4.07 ns/iter   4.19 ns  ▇             █
                       (3.89 ns … 40.75 ns)   4.29 ns  █             █
                    (123.05  b …  78.63 kb) 138.08  b ▂█▆▂▁▁▁▁▁▁▁▁▁▁▂██▂▁▁▁
isCore2("foo")                86.79 ns/iter  88.56 ns █
                     (84.39 ns … 112.85 ns)  94.60 ns █▇▄
                    (130.86  b …  69.39 kb) 174.89  b ███▆▄▃▃▃▅▃▃▂▇▅▄▂▁▁▁▁▁
isCore2b("foo")                4.68 ns/iter   4.65 ns     █
                       (4.30 ns … 31.97 ns)   5.91 ns     █
                    (134.77  b …  57.55 kb) 138.93  b ▁▁▁▁█▃▁▂▁▁▁▁▁▁▁▁▁▁▁▁▁
isCore3("foo")                 5.48 ns/iter   5.63 ns ▂      █
                       (5.23 ns … 27.34 ns)   6.37 ns █      █
                    (134.77  b …  26.34 kb) 135.78  b █▅▁▁▁▁▁█▃▁▁▁▁▁▁▁▁▁▁▁▁

summary
  isCore1("foo")
   1.15x faster than isCore2b("foo")
   1.35x faster than isCore3("foo")
   21.31x faster than isCore2("foo")

Is this premature optimization? Might be. I added the following to a RITM install to count the number of calls to isCore.

const isCoreImpl = isCore
let isCoreCount = 0
isCore = (moduleName) => {
    isCoreCount++
    return isCoreImpl(moduleName);
}
setInterval(() => {
    console.log('isCoreCount:', isCoreCount);
}, 3000).unref();

I ran a minimal Express app instrumented with OTel. The result was 578 calls to isCore(). Times 87ns, we are talking 50 microseconds. On a bigger app with, say, 10x the number of JS files we still are less than a millisecond, so yah, probably premature optimization.

If we want it, the isCore2b above is an alternative:

const builtinModulesMap = {}
Module.builtinModules.forEach(k => { builtinModulesMap[k] = true; })
const isCore2b = moduleName => {
  if (moduleName.startsWith('node:')) {
    return true
  }
  return !!builtinModulesMap[moduleName]
}

[timfish]

I'm in two minds as to whether it's worth it.

I'd guess if you're really concerned about performance, you're likely not sticking around on Node v14!

[timfish]

I changed this to use a Set<string> 379650f

[trentm]

[email protected] published with this change.