feat: upgrade to Istanbul 2.0 API, introduce nyc bin

[bcoe]

Upgrades to the Istanbul 2.0 API (addressing the issue discussed in https://github.com/istanbuljs/nyc/issues/373).

Pulls in the nyc CLI tool to the project itself, and documents the usage of the tool for folks using browserify-istanbul.

reviewers: @alexindigo (hey Alex!), @devongovettl, @alexjeffburke.

Note: this should probably be a major version bump, as Istanbul 2.0 has some slight variations with regards to how it instruments files.

[alexindigo]

Hey @bcoe :)

Thanks for the PR. Looks fancy. :)

Just to make sure I got it correctly, it would still work with istanbul@0.x right?

[bcoe]

@alexindigo we should test to see the behavior of the 0.x bin, unfortunately Istanbul 2.0 has some slight changes in instrumentation that might cause issues -- making this potentially a major bump.

[bcoe]

the advantages of the 2.x API are:

• bug fixes for some long standing instrumentation bugs.
• first-class ES2015 support.

[alexindigo]

Oh man, is it a fork of istanbul? o_0

[bcoe]

@alexindigo it's a ground up rewrite by @gotwarlost, the developer who wrote Istanbul -- In addressing long standing issues, addressing topics like ES6 support, he decided to split up Istanbul into many modules -- istanbul-lib-instrument is the instrumentation bit.

[alexindigo]

I see. Thanks.

Where I'm going from – I see this tool as helper to istanbul lib found on npm, and it requires developers to install both. So if npm version of istanbul is still 0.4 and we'd bump this one to newer API, devs wouldn't be able to use it. Or did I miss something?

[bcoe]

@alexindigo I'm doing testing, it looks like the istanbul 2.0 API should not break the old reporter:

I'm testing using @alexjeffburke's project:

> phantomjs ./node_modules/mocha-phantomjs-core/mocha-phantomjs-core.js test/coverage.html spec "`node -pe 'JSON.stringify({useColors:true,hooks:"mocha-phantomjs-istanbul"})'`" && istanbul report lcov text-summary && echo open coverage/lcov-report/index.html

The format of the coverage JSON output by Istanbul 2.x vs. Istanbul 0.x has not changed significantly.

tldr; going forward, it's good for people to start moving to the 2.x API, which is currently supported by the nyc bin, and is in more active development. However, the 2.x instrumenter shouldn't be creating output that's incompatible with the old Istanbul binary.

I recommend testing against several canary in the coal mine projects though.

[alexindigo]

Oh, so new 2.x API is only available with nyc for now? Is there a plan to merge it back into istanbul? I'm just thinking how to keep it developer-friendly, if nyc is departing from istanbul, maybe we need browserify-nyc – it will be more clear than to say "Please use browserify-istanbul version 3.x and up with your nyc installation" :)

I'll test it with some of my projects.

[bcoe]

@alexindigo see https://github.com/gotwarlost/istanbul/issues/706, and https://github.com/istanbuljs/nyc/issues/309 for the current state of the world.

tldr; it seems to make sense to merge the two projects eventually, but most of my OSS time recently has been spent maintaining nyc.

[alexindigo]

Sorry for the delay. I found where it breaks things for me. This repo: https://github.com/alexindigo/obake

> obake@0.1.2 test /Users/alex/Projects/obake
> tape test/test-*.js

TAP version 13
# instrumented, no coverage
ok 1 running instrumented obake should not error
ok 2 should get number of covered statements, even if number is 0
ok 3 should get number of covered branches, even if number is 0
ok 4 should get number of covered functions, even if number is 0
ok 5 should get number of covered lines, even if number is 0
not ok 6 should have 20 statements covered
  ---
    operator: equal
    expected: '20'
    actual:   '22'
    at: spawner (/Users/alex/Projects/obake/test/spawner.js:32:5)
  ...
not ok 7 out of 20 statements observed
  ---
    operator: equal
    expected: '20'
    actual:   '22'
    at: spawner (/Users/alex/Projects/obake/test/spawner.js:32:5)
  ...
ok 8 should have 4 branches covered
ok 9 out of 4 branches observed
ok 10 should have 4 functions covered
ok 11 out of 4 functions observed
not ok 12 should have 20 lines covered
  ---
    operator: equal
    expected: '20'
    actual:   '22'
    at: spawner (/Users/alex/Projects/obake/test/spawner.js:32:5)
  ...
not ok 13 out of 20 lines observed
  ---
    operator: equal
    expected: '20'
    actual:   '22'
    at: spawner (/Users/alex/Projects/obake/test/spawner.js:32:5)
  ...
# instrumented, no coverage
ok 14 running instrumented obake should not error
ok 15 should get number of covered statements, even if number is 0
ok 16 should get number of covered branches, even if number is 0
ok 17 should get number of covered functions, even if number is 0
ok 18 should get number of covered lines, even if number is 0
ok 19 should have 0 statements covered
ok 20 out of 0 statements observed
ok 21 should have 0 branches covered
ok 22 out of 0 branches observed
ok 23 should have 0 functions covered
ok 24 out of 0 functions observed
ok 25 should have 0 lines covered
ok 26 out of 0 lines observed

1..26
# tests 26
# pass  22
# fail  4

Checking if it's me or the changes.

[alexindigo]

This is what it produces:

[ 'Statements   : 100% ( 22/22 )',
  '22',
  '22',
  index: 82,
  input: '\n=============================== Coverage summary ===============================\nStatements   : 100% ( 22/22 )\nBranches     : 100% ( 4/4 )\nFunctions    : 100% ( 4/4 )\nLines        : 100% ( 22/22 )\n================================================================================\nDone\n' ]

And this is the coverage file: https://gist.github.com/alexindigo/5553733b5a84f9504644e093922296d3

[alexindigo]

I double checked and coverage file is the same for both (old and new browserify-istanbul).

[alexindigo]

Somehow I missed this part in the beginning – https://github.com/istanbuljs/istanbul-lib-instrument/blob/master/v0-changes.md

Now it makes more sense. So I think it's the way everything is going anyway, and we need to bite the bullet and publish major version.

Just couple more things, with istanbul-lib-instrument can we use it from nyc? I assume it has one in the dependencies. Instead of making it a dependency for people who don't use it (is looks like optional thing in the code). This package is meant to be just a wrapper for browserify and istanbul you already have in your setup. Would it work for you if we go same route with new API or there is more to it? And another thing, just curios what your thoughts on using util._extend instead of object-assign as separate dependency. Thank you.

@bcoe ^