Repository: istanbuljs/nyc
Pull request #1179 (2019-09-14 12:41:00)

fix: Honor eager setting (false by default)
This delays loading of istanbul-lib-instrument until it is needed.

---

This fixes a performance regression caused by #985, adds tests to prevent in the future.  To test you can create a script `eager.js`:
```js
console.log(Object.keys(require.cache).length - 1); // don't count eager.js itself
```

Then you can test the effect of non-eager loading:
```
$ node eager.js
0
$ ./bin/nyc.js --silent --exclude=eager.js --eager=true node eager.js
293
$ ./bin/nyc.js --silent --exclude=eager.js node eager.js
143
```

Running under nyc with `eager: true` (or without this patch) results in 293 additional modules being loaded, this patch by default reduces the "always load" to 143.  This is demonstrated here by ensuring that all sources are excluded from instrument but in practice large test suites that load sources from multiple processes would see benefit when caching-transform bypasses repeated instruments.

coveralls: 
[![Coverage Status](https://coveralls.io/builds/25714903/badge)](https://coveralls.io/builds/25714903)

Coverage increased (+0.2%) to 98.715% when pulling **25730d5648e4dbc4acf501476316e95c6374013d on coreyfarrell:fix-eager** into **03ba5b9cb0a0acc7451d3dd0b8fc4241fc6d35d3 on istanbuljs:master**.


Patch:
diff --git a/lib/instrumenters/istanbul.js b/lib/instrumenters/istanbul.js
index a10c7d1..2946218 100644
--- a/lib/instrumenters/istanbul.js
+++ b/lib/instrumenters/istanbul.js
@@ -1,10 +1,10 @@
 'use strict'
 
-const { createInstrumenter } = require('istanbul-lib-instrument')
-const convertSourceMap = require('convert-source-map')
-const mergeSourceMap = require('merge-source-map')
-
 function InstrumenterIstanbul (options) {
+  const { createInstrumenter } = require('istanbul-lib-instrument')
+  const convertSourceMap = require('convert-source-map')
+  const mergeSourceMap = require('merge-source-map')
+
   const { plugins } = options
   const configPlugins = plugins ? { plugins } : {}
 
@@ -28,6 +28,7 @@ function InstrumenterIstanbul (options) {
       // TODO: test source-map merging logic.
       if (options.produceSourceMap) {
         var lastSourceMap = instrumenter.lastSourceMap()
+        /* istanbul ignore else */
         if (lastSourceMap) {
           if (sourceMap) {
             lastSourceMap = mergeSourceMap(
diff --git a/lib/instrumenters/noop.js b/lib/instrumenters/noop.js
index 7efe8bd..b0c9f70 100644
--- a/lib/instrumenters/noop.js
+++ b/lib/instrumenters/noop.js
@@ -1,6 +1,8 @@
-const { readInitialCoverage } = require('istanbul-lib-instrument')
+'use strict'
 
 function NOOP () {
+  const { readInitialCoverage } = require('istanbul-lib-instrument')
+
   return {
     instrumentSync (code, filename) {
       const extracted = readInitialCoverage(code)
diff --git a/lib/source-maps.js b/lib/source-maps.js
index 89d0796..d620725 100644
--- a/lib/source-maps.js
+++ b/lib/source-maps.js
@@ -4,64 +4,65 @@ const libSourceMaps = require('istanbul-lib-source-maps')
 const fs = require('fs')
 const path = require('path')
 
-function SourceMaps (opts) {
-  this.cache = opts.cache
-  this.cacheDirectory = opts.cacheDirectory
-  this.loadedMaps = {}
-  this._sourceMapCache = libSourceMaps.createSourceMapStore()
-}
+class SourceMaps {
+  constructor (opts) {
+    this.cache = opts.cache
+    this.cacheDirectory = opts.cacheDirectory
+    this.loadedMaps = {}
+    this._sourceMapCache = libSourceMaps.createSourceMapStore()
+  }
 
-SourceMaps.prototype.cachedPath = function (source, hash) {
-  return path.join(
-    this.cacheDirectory,
-    `${path.parse(source).name}-${hash}.map`
-  )
-}
+  cachedPath (source, hash) {
+    return path.join(
+      this.cacheDirectory,
+      `${path.parse(source).name}-${hash}.map`
+    )
+  }
 
-SourceMaps.prototype.purgeCache = function () {
-  this._sourceMapCache = libSourceMaps.createSourceMapStore()
-  this.loadedMaps = {}
-}
+  purgeCache () {
+    this._sourceMapCache = libSourceMaps.createSourceMapStore()
+    this.loadedMaps = {}
+  }
 
-SourceMaps.prototype.extractAndRegister = function (code, filename, hash) {
-  var sourceMap = convertSourceMap.fromSource(code) || convertSourceMap.fromMapFileSource(code, path.dirname(filename))
-  if (sourceMap) {
-    if (this.cache && hash) {
-      const mapPath = this.cachedPath(filename, hash)
-      fs.writeFileSync(mapPath, sourceMap.toJSON())
-    } else {
-      this._sourceMapCache.registerMap(filename, sourceMap.sourcemap)
+  extractAndRegister (code, filename, hash) {
+    const sourceMap = convertSourceMap.fromSource(code) || convertSourceMap.fromMapFileSource(code, path.dirname(filename))
+    if (sourceMap) {
+      if (this.cache && hash) {
+        const mapPath = this.cachedPath(filename, hash)
+        fs.writeFileSync(mapPath, sourceMap.toJSON())
+      } else {
+        this._sourceMapCache.registerMap(filename, sourceMap.sourcemap)
+      }
     }
+    return sourceMap
   }
-  return sourceMap
-}
 
-SourceMaps.prototype.remapCoverage = function (obj) {
-  var transformed = this._sourceMapCache.transformCoverage(
-    libCoverage.createCoverageMap(obj)
-  )
-  return transformed.map.data
-}
+  remapCoverage (obj) {
+    const transformed = this._sourceMapCache.transformCoverage(
+      libCoverage.createCoverageMap(obj)
+    )
+    return transformed.map.data
+  }
 
-SourceMaps.prototype.reloadCachedSourceMaps = function (report) {
-  Object.keys(report).forEach((absFile) => {
-    var fileReport = report[absFile]
-    if (fileReport && fileReport.contentHash) {
-      var hash = fileReport.contentHash
-      if (!(hash in this.loadedMaps)) {
-        try {
-          const mapPath = this.cachedPath(absFile, hash)
-          this.loadedMaps[hash] = JSON.parse(fs.readFileSync(mapPath, 'utf8'))
-        } catch (e) {
-          // set to false to avoid repeatedly trying to load the map
-          this.loadedMaps[hash] = false
+  reloadCachedSourceMaps (report) {
+    Object.entries(report).forEach(([absFile, fileReport]) => {
+      if (fileReport && fileReport.contentHash) {
+        const hash = fileReport.contentHash
+        if (!(hash in this.loadedMaps)) {
+          try {
+            const mapPath = this.cachedPath(absFile, hash)
+            this.loadedMaps[hash] = JSON.parse(fs.readFileSync(mapPath, 'utf8'))
+          } catch (e) {
+            // set to false to avoid repeatedly trying to load the map
+            this.loadedMaps[hash] = false
+          }
+        }
+        if (this.loadedMaps[hash]) {
+          this._sourceMapCache.registerMap(absFile, this.loadedMaps[hash])
         }
       }
-      if (this.loadedMaps[hash]) {
-        this._sourceMapCache.registerMap(absFile, this.loadedMaps[hash])
-      }
-    }
-  })
+    })
+  }
 }
 
 module.exports = SourceMaps
diff --git a/npm-run-clean.js b/npm-run-clean.js
new file mode 100644
index 0000000..589dac3
--- /dev/null
+++ b/npm-run-clean.js
@@ -0,0 +1,19 @@
+#!/usr/bin/env node
+'use strict'
+
+const { promisify } = require('util')
+const rimraf = promisify(require('rimraf'))
+
+Promise.all([
+  '**/.nyc_output',
+  'node_modules/.cache',
+  '.self_coverage',
+  'test/**/.cache',
+  'test/fixtures/cli/coverage',
+  'test/fixtures/cli/fakebin/node',
+  'test/fixtures/cli/fakebin/npm',
+  'test/fixtures/cli/foo-cache',
+  'test/fixtures/cli/nyc-config-js/node_modules',
+  'test/temp-dir-*',
+  'self-coverage'
+].map(f => rimraf(f, { cwd: __dirname })))
diff --git a/package.json b/package.json
index e348aea..e0d9673 100644
--- a/package.json
+++ b/package.json
@@ -9,7 +9,7 @@
     "test": "tap",
     "snap": "npm test -- --snapshot",
     "posttest": "npm run report",
-    "clean": "rimraf ./.nyc_output ./node_modules/.cache ./.self_coverage ./test/fixtures/.nyc_output ./test/fixtures/node_modules/.cache ./test/fixtures/cli/foo-cache ./test/temp-dir-* ./self-coverage",
+    "clean": "node ./npm-run-clean.js",
     "instrument": "node ./build-self-coverage.js",
     "report": "node ./bin/nyc report --temp-dir ./.self_coverage/ -r text -r lcov",
     "release": "standard-version"


Test patch:
diff --git a/tap-snapshots/test-eager.js-TAP.test.js b/tap-snapshots/test-eager.js-TAP.test.js
new file mode 100644
index 0000000..7949315
--- /dev/null
+++ b/tap-snapshots/test-eager.js-TAP.test.js
@@ -0,0 +1,16 @@
+/* IMPORTANT
+ * This snapshot file is auto-generated, but designed for humans.
+ * It should be checked into source control and tracked carefully.
+ * Re-generate by setting TAP_SNAPSHOT=1 and running tests.
+ * Make sure to inspect the output below.  Do not ignore changes!
+ */
+'use strict'
+exports[`test/eager.js TAP eager disabled by default > stdout 1`] = `
+0
+
+`
+
+exports[`test/eager.js TAP eager enabled > stdout 1`] = `
+1
+
+`
diff --git a/test/eager.js b/test/eager.js
new file mode 100644
index 0000000..14ccddb
--- /dev/null
+++ b/test/eager.js
@@ -0,0 +1,30 @@
+'use strict'
+
+const path = require('path')
+
+const t = require('tap')
+
+const { testSuccess } = require('./helpers')
+
+const cwd = path.resolve(__dirname, 'fixtures')
+
+t.test('eager disabled by default', t => testSuccess(t, {
+  args: [
+    '--silent=true',
+    '--exclude=eager.js',
+    process.execPath,
+    './eager.js'
+  ],
+  cwd
+}))
+
+t.test('eager enabled', t => testSuccess(t, {
+  args: [
+    '--silent=true',
+    '--eager=true',
+    '--exclude=eager.js',
+    process.execPath,
+    './eager.js'
+  ],
+  cwd
+}))
diff --git a/test/fixtures/eager.js b/test/fixtures/eager.js
new file mode 100644
index 0000000..473b9ff
--- /dev/null
+++ b/test/fixtures/eager.js
@@ -0,0 +1,5 @@
+#!/usr/bin/env node
+'use strict'
+
+const lib = require.resolve('istanbul-lib-instrument')
+console.log(Object.keys(require.cache).filter(s => s === lib).length)