feat: add cwd option to es6+ instrument command
#1024
Conversation
- Notes:
Add instrument command support for the 'cwd' option
Refactor nyc-integration instrument tests so tests with output folder specified aren't in the no output folder suite
Refactor instrument.js and instrumentAllFiles command to use modern javascript
Remove call to deprecated 'process.exit()' in instrument command
Use file extension as an index to find the correct transform, rather than trying to match a transform name to the rightmost characters of a filename
* There is another one of these in the code base but it's out of the scope of this PR
BTW, I'm not going to widen the scope of this PR, this is it!
- Assumptions:
If you're instrumenting a single file cwd/a/b/d.js, and specifying an output directory cwd/e, the result will be cwd/e/d.js
That we want to use file extensions for find transforms, the existing implementation would use the '.js' instrumenter for both '.js' and '.mjs' files, these changes assume that was wrong.
That the filename in the covered file should be of the form './path/to/file.js'
This is intended to sit on top of the nyc es2015+ commit. It uses the cwd option and its defaults in nyc to make paths more consistent.
lib/commands/instrument.js
Outdated
| @@ -28,6 +29,10 @@ exports.builder = function (yargs) { | |||
| type: 'boolean', | |||
| description: "should nyc's instrumenter produce source maps?" | |||
| }) | |||
| .option('cwd', { | |||
There was a problem hiding this comment.
cwd is actually already defined in lib/config-util.js in a way that allows it to be global. I think we should not add the .option to this file but we should update the describe string in lib/config-util.js. Note the current definition of lib/config-util.js often does not result in cwd: process.cwd(). It uses find-up to search for the package.json which owns process.cwd(), then uses the dirname of that package.json.
Existing: working directory used when resolving paths
Maybe change to: base directory of this project
The important thing is that --cwd effects the interpretation of include and exclude. I think it should not effect the interpretation of the input/output positional arguments. Consider this, when you run nyc --cwd=.. ./somescript.js the ./somescript.js is found from $PWD, not from the value of --cwd.
index.js
Outdated
| @@ -193,15 +193,15 @@ NYC.prototype.addAllFiles = function () { | |||
| NYC.prototype.instrumentAllFiles = function (input, output, cb) { | |||
| let inputDir = '.' + path.sep | |||
| const visitor = filename => { | |||
| const inFile = path.resolve(inputDir, filename) | |||
| const inFile = path.resolve(this.cwd, inputDir, filename) | |||
There was a problem hiding this comment.
My gut reaction is that we these changes to index.js should not happen.
All changed, so all this does now is propagate `cwd` so that it can influence `testExclude` and `require` path settings.
This adds support and tests for the '--cwd' option for the instrument command, it's intended to sit on top of the recent es6+ changes.