fix: source was being instrumented twice, due to upstream fix in ista…

[bcoe]

BREAKING CHANGE: --hook-run-in-context, and --hook-run-in-this-context are no longer true by default (they should be enabled if you're using a library like requirejs).

see: istanbuljs/istanbuljs#179, #852

CC: @coreyfarrell

[coreyfarrell]

Since this is a breaking change do we need to leave the empty _wrapRequire here? Nothing in nyc uses it after this patch.

[coreyfarrell]

Apparently github won't let me comment on lines you did not modify. For any testing I updated _handleJs:

NYC.prototype._handleJs = function (code, options) {
  if (typeof options != 'object' || typeof options.filename !== 'string') {
    throw new TypeError('options.filename must be a string')
  }
  var filename = options.filename
  /* Remaining original function body here.. */
}

No exceptions were detected by npm test. I don't think the throw new TypeError() is needed or wanted IRL, path.relative will throw an exception if options.filename is not a string.

[coreyfarrell]

I tested removal of this one line, created a tarball locally, pointed a test package.json to the tarball and nvm exec 6 npm install worked without an issue.

Since this fixes nyc@12 for node@6 should it be done separate (first) so a non-breaking [email protected] can be released? I'll have some additional patches for both nyc and istanbuljs repos which I hope can be considered before next major release just in case any of my changes are semver-major.

[coreyfarrell]

This test still failed for me. I had to also add the argument '--hook-require=false' for this test to pass.

[bcoe]

@coreyfarrell I believe I've addressed your review. Mind verifying that everything works for you as expected? any other changes you think we'll need to make to address your issue?

[coreyfarrell]

@bcoe the only thing remaining to make this compatible with istanbul-lib-hook@next is to take the options argument at _handleJs and var filename = options.filename, but then it'll be incompatible with istanbul-lib-hook@latest. Honestly I think [email protected] should be 2.0.0 since it is a breaking change but that's up to you. The branches I've been testing with are at https://github.com/coreyfarrell/nyc/tree/require-hook-collision and https://github.com/coreyfarrell/istanbuljs/tree/update-deps. I plan on submitting PR's for those once this is merged.

One thing I just noticed the PR title and commit message are inaccurate, it's actually a change in [email protected] (still next) which causes the issue.

[bcoe]

@coreyfarrell if you go ahead and get everything working appropriately, we can change the tag on next to be 2.0.0 rather than 1.3.0; I'm fine with this.

[coreyfarrell]

@bcoe I've posted istanbuljs/istanbuljs#180 and #855. The nyc test fails in CI due to not testing against my istanbuljs PR.

My PR's don't bump the version of any packages since you do that in separate 'publish' commits. I think the istanbuljs dependencies will need to be updated to point to latest next once a new publish is performed, then I can update my nyc PR to require those updated istanbuljs versions and it'll pass the tests (though admittedly I've never tested with appveyor so I don't know what's going on there).

[bcoe]

@coreyfarrell this is now published to nyc@next please let me know if things are working properly for you.

[coreyfarrell]

Seems to work for me. I verified npm i using node.js 6 and 10. nyc ran fine in node.js 8 and 10 (my project doesn't support node.js 6). Worth noting npm test in nyc & istanbuljs work things much harder than my little projects.