feat: nyc is being refactored to become the official Istanbul 1.0 bin #286

bcoe · 2016-06-26T23:09:22Z

This refactoring work moves nyc towards using Istanbul's more modular v1.0 API.

Features of Note

I've changed the messaging somewhat, announcing nyc as the official Istanbul bin.
We no longer call out to the istanbul bin for the feature check-coverage (calculating this is now our responsibility).
we no longer load .istanbul.yml, we will extend nyc's configuration gradually to support any configuration options that we're missing.
we now point folks towards babel-plugin-istanbul for ES6/ES7 support.
we no longer use the preamble function for --all functionality, using an approach closer to what istanbul's original bin used:
- this fixes Implement --all without requiring modules #183

Breaking Changes

.istanbul.yaml is no longer supported.
removed instrumenter configuration option for the time being (I think we get pretty far with the new istanbul-lib-instrument, and a noop instrumenter, and can avoid abstracting this part of the API for the time being).
completely new underlying instrumentation engine.

Todo

test with a few libraries on Windows (windows testing went very smoothly \o/)
add unit tests for native ES6 code (excited to exercise the new instrumentation library).
create tickets for further work: porting more istanbul configuration options, switching to the istanbuljs org's source-map library, source-maps for errors?
get [email protected] published.
~~- [ ] get --all working with babel-plugin-istanbul.~~ we can address this separately._
consolidate source-map support.

CC: @novemberborn, @jamestalmage, @dtinth, @gotwarlost

bcoe · 2016-06-27T00:25:35Z

one thing I've noted, is that the final bundled nyc balloons from 3MB to 7MB. On the plus side, since it's bundled it comes down as a single .tgz file, on the other hand, it would definitely be good to try to shrink the module a little bit.

As we discussed, a good chunk of this size is in istanbul-lib-instrument:

16K append-transform
 16K    arrify
164K    caching-transform
 72K    convert-source-map
 52K    default-require-extensions
 56K    find-cache-dir
 68K    find-up
560K    foreground-child
308K    glob
108K    istanbul-lib-coverage
 57M    istanbul-lib-instrument
384K    istanbul-lib-report
5.4M    istanbul-reports
 52K    md5-hex
672K    micromatch
156K    mkdirp
 16K    pkg-up
 16K    resolve-from
 24K    rimraf
 24K    signal-exit
784K    source-map
304K    spawn-wrap
1.0M    test-exclude
1.9M    yargs

Specifically the babel tool-chain:

 12M    ./babel-generator
 12M    ./babel-template
 13M    ./babel-traverse
 12M    ./babel-types
7.8M    ./babylon

Currently we bundle nyc with an [email protected] bin, to avoid some weird historical bugs related to singletons -- I'll try publishing a version using deduping, and see where it gets us to; perhaps we'd do better to address these long-standing singleton bugs, rather than increasing the size of the module.

bcoe · 2016-06-27T00:41:24Z

package size update

If I bundle using an [email protected] bin, the tarball size is only 2.7MB, and deduping gets the total size on disk down to about 25MB (post untar) not too shabby!

testing update

I've published the new [email protected] bin to the next tag, I would love help testing. If folks could run it against a few of their modules.

npm cache clear; npm i nyc@next

CC: @JaKXz, @rmg, @addaleax.

bcoe · 2016-06-27T02:31:52Z

I'm seeing some issues with the ava test suite, using the new instrumentation approach:

not ok should match pattern provided
    found: >
      /Users/benjamincoe/bcoe/coverage/ava/node_modules/nyc/node_modules/babylon/lib/parser/location.js:25
        throw err;
        ^

      SyntaxError: 'return' outside of function (14:1)
          at Parser.pp.raise (/Users/benjamincoe/bcoe/coverage/ava/node_modules/nyc/node_modules/babylon/lib/parser/location.js:22:13)
          at Parser.pp.parseReturnStatement (/Users/benjamincoe/bcoe/coverage/ava/node_modules/nyc/node_modules/babylon/lib/parser/statement.js:321:10)
          at Parser.pp.parseStatement (/Users/benjamincoe/bcoe/coverage/ava/node_modules/nyc/node_modules/babylon/lib/parser/statement.js:107:19)
          at Parser.pp.parseBlockBody (/Users/benjamincoe/bcoe/coverage/ava/node_modules/nyc/node_modules/babylon/lib/parser/statement.js:529:21)
          at Parser.pp.parseBlock (/Users/benjamincoe/bcoe/coverage/ava/node_modules/nyc/node_modules/babylon/lib/parser/statement.js:510:8)
          at Parser.pp.parseStatement (/Users/benjamincoe/bcoe/coverage/ava/node_modules/nyc/node_modules/babylon/lib/parser/statement.js:127:19)
          at Parser.pp.parseIfStatement (/Users/benjamincoe/bcoe/coverage/ava/node_modules/nyc/node_modules/babylon/lib/parser/statement.js:314:26)
          at Parser.pp.parseStatement (/Users/benjamincoe/bcoe/coverage/ava/node_modules/nyc/node_modules/babylon/lib/parser/statement.js:105:19)
          at Parser.pp.parseBlockBody (/Users/benjamincoe/bcoe/coverage/ava/node_modules/nyc/node_modules/babylon/lib/parser/statement.js:529:21)
          at Parser.pp.parseTopLevel (/Users/benjamincoe/bcoe/coverage/ava/node_modules/nyc/node_modules/babylon/lib/parser/statement.js:36:8)

@jamestalmage @novemberborn any thoughts?

jamestalmage · 2016-06-27T03:33:21Z

It looks like you probably need to enable the allowReturnOutsideFunction in babylon.

bcoe · 2016-06-27T03:57:51Z

@jamestalmage just came to that conclusion myself, got tests running by removing returns in the top-level cli module, I'll go ahead and set that configuration option. great find!

kentcdodds · 2016-06-27T04:18:49Z

index.js

+      instrumented = instrumenter.instrumentSync(code, filename)
+    } catch (e) {
+      // don't fail external tests due to instrumentation bugs.
+      console.warn('failed to instrument', filename, 'with error:', e.message)


Any way to configure this to prevent it from logging? Could there be a scenario where this happens and cannot be easily prevented? If so, this log would be annoying/distracting noise.

@kentcdodds my gut was annoy people while we're in the early phases of v7, get bug reports, and fix quickly -- there are a few more knobs and dials it looks like we can turn in babylon.

If you think this strategy might annoy people too much, I could definitely be convinced to drop the log message.

Let's wait until it becomes a problem I think 👍

bcoe · 2016-06-27T06:18:10Z

added a test for native ES6 support, found a few interesting edge-cases but in general, things seem to be working a lot better:

I could not get this to instrument:

let a = 33 || () => {
   console.log('missed')
}

CC: @gotwarlost ^ we should probably put a test around this.

JaKXz · 2016-06-27T06:50:30Z

@bcoe just tried [email protected] and found some reporting errors with an ES2015 Babel project.

Interestingly, the errors look similar to what I was getting with the current instructions using the --all flag with the __coverage__ plugin. I ignored this before because of the --all flag and it's existing issues.

gotwarlost · 2016-06-27T14:30:13Z

On the subject of allowReturnsOutsideFunction, this is controlled using the instrumenter option autoWrap (false by default - which is a change from v0)

https://github.com/istanbuljs/istanbul-lib-instrument/blob/master/v0-changes.md

gotwarlost · 2016-06-27T14:33:40Z

On the es6 edge cases, this is a babylon issue:

$ cat .babelrc 
{
  "presets": ["es2015"]
}

$ ./node_modules/.bin/babel 
let a = 33 || () => {
   console.log('missed')
}
SyntaxError: unknown: Unexpected token (1:15)
> 1 | let a = 33 || () => {
    |                ^
  2 |    console.log('missed')
  3 | }
  4 |

gotwarlost · 2016-06-27T14:36:03Z

@bcoe - great progress so far. We need to figure out - if the istanbul-cli is gone - what happens to commands needed for testing on the browser (e.g. istanbul instrument , istanbul report etc.)

gotwarlost · 2016-06-27T14:54:21Z

I think babylon is correct in its parsing and parens are needed around the second expression (need someone who knows the ES6 spec intimately to chime in). This one works:

let a = 33 || (() => {
   console.log('missed')
})

bcoe · 2016-06-27T15:51:10Z

@JaKXz mind opening a separate issues with a reproduction case for the behavior that you're seeing?
@gotwarlost found the allowReturnsOutsideFunction setting and enabled it 👍, I think in nyc's case we want to be pretty loose out of the gate (we'll probably find there are a few other babylon nits we gradually address).
@gotwarlost embarrassingly I haven't used Istanbul too much in the browser, mind opening a ticket describing an MVP of some of the features we should consider adding:
- we have nyc report.
- we can add nyc instrument, which I assume instruments a file and returns the code.
@gotwarlost mind tossing me on babel-plugin-istanbul as an owner:
- I'd like to mention this plugin on HackerNews, I think it will bring some attention to the Istanbul 1.0 effort.
- once we land this, and are happy, I'd love to also write a blog post on the merger of Istanbul 1.0 + nyc.

addaleax · 2016-06-27T19:48:21Z

Ran tests for a couple of my packages with nyc@next, looks great so far! :)

bcoe · 2016-06-28T05:05:26Z

I'm a little afraid to actually drop the hammer, but:

I've tested thoroughly with Windows.
I've tested against yargs, ava, npm, and tap (found a few bugs in the process, but for the most part all these large nyc reliant libraries are working well).
people's smoke tests seem to be going well so far?

If folks could take one more day to hammer on this (CC: @istanbuljs/nyc) I think we'll land it.

kentcdodds · 2016-06-28T17:00:06Z

After npm i nyc@next, I'm getting:

/Users/kdodds/Developer/paypal/p2pnodeweb/node_modules/nyc/node_modules/istanbul-reports/lib/lcovonly/index.js:32
        writer.println('FN:' + [meta.decl.start.line, meta.name].join(','));
                                         ^

TypeError: Cannot read property 'start' of undefined
    at /Users/kdodds/Developer/paypal/p2pnodeweb/node_modules/nyc/node_modules/istanbul-reports/lib/lcovonly/index.js:32:42
    at Array.forEach (native)
    at LcovOnlyReport.onDetail (/Users/kdodds/Developer/paypal/p2pnodeweb/node_modules/nyc/node_modules/istanbul-reports/lib/lcovonly/index.js:30:28)
    at LcovReport.(anonymous function) [as onDetail] (/Users/kdodds/Developer/paypal/p2pnodeweb/node_modules/nyc/node_modules/istanbul-reports/lib/lcov/index.js:21:24)
    at Visitor.(anonymous function) [as onDetail] (/Users/kdodds/Developer/paypal/p2pnodeweb/node_modules/nyc/node_modules/istanbul-lib-report/lib/tree.js:34:30)
    at ReportNode.Node.visit (/Users/kdodds/Developer/paypal/p2pnodeweb/node_modules/nyc/node_modules/istanbul-lib-report/lib/tree.js:123:17)
    at /Users/kdodds/Developer/paypal/p2pnodeweb/node_modules/nyc/node_modules/istanbul-lib-report/lib/tree.js:116:23
    at Array.forEach (native)
    at visitChildren (/Users/kdodds/Developer/paypal/p2pnodeweb/node_modules/nyc/node_modules/istanbul-lib-report/lib/tree.js:115:32)
    at ReportNode.Node.visit (/Users/kdodds/Developer/paypal/p2pnodeweb/node_modules/nyc/node_modules/istanbul-lib-report/lib/tree.js:126:5)

Works fine with latest.

bcoe · 2016-06-29T15:03:01Z

@kentcdodds don't suppose you might be able to make a reproduction case, and tack it onto an issue? I'm holding off on merging this until we address a couple similar issues that have cropped up.

@gotwarlost saw some patches landing regarding crashers/performance. Just let me know any code reviews/implementation work you need done.

kentcdodds · 2016-06-29T15:06:05Z

I believe my issue is the same that @JaKXz reported in #288

gotwarlost · 2016-06-29T17:33:14Z

istanbul v1 now adds a new location called decl in the fnMap metadata to track the range for function declarations. This is being lost here: https://github.com/istanbuljs/nyc/blob/master/lib/source-map-cache.js#L123-L147

That's the reason the lcov report is failing. Note that this new location info also needs to be mapped when source maps are in play.

bcoe · 2016-06-29T17:49:52Z

@gotwarlost would retiring nyc's sourcemap handling for istanbul's solve this issue?

gotwarlost · 2016-06-29T18:00:42Z

possibly. We need to compare if there is any difference in processing between the two.

…relying on the preamble

… for the actual bump

…ly by old Istanbul

bcoe · 2016-07-02T21:55:49Z

@gotwarlost I pulled in istanbul-lib-source-maps and it seems to have solved the problems, there's still the weird highlighting issues (which I think you have an open issue on babel regarding). We have a few remaining things to do:

seems like there continues to be some performance questions with babel-plugin-istanbul.
I'd like to add the instrument feature to nyc so that we can support front-end better, like you requested. Mind commenting on this issue and letting me know if I'm on the right track.
I'd like to switch to using your better --all support (maybe this will make it easier to support --all in babel-pluglin-istanbul too?).
it would be nice to dig into merging istanbul-lib-hook, and append-transform's functionality, as it relates to nyc.

Other than that, I think we're pretty on track for landing this?

gotwarlost · 2016-07-03T05:23:43Z

Re: my "better --all" support, I'm having second thoughts about this. It may not be "better" :)

Problem is the babel cache. If we process a file for side-effects only then only the preamble gets written to the babel cache because we basically nuke the real code, which is going to have all sorts of fun implications when the user goes about exercising that code later and "nothing works".

If the caching were smarter to allow inclusion of the extra parameters in the cache key, it would be fine but previous comments seemed to indicate that the caching in babel is naive.

jamestalmage · 2016-07-06T01:54:51Z

I have been poking around in the babel code recently. I think the cache only applies to babel-register. You can still parse any code using babel-core without impacting the cache.

gotwarlost · 2016-07-10T04:05:32Z

@jamestalmage - when the babel plugin is used, it does not control any babel-specific options (the library API will not have issues since we use babylon and babel-generate as libraries). I'm not sure what will happen to the cache when babel-plugin-istanbul is used.

gotwarlost · 2016-07-10T04:07:20Z

Although if we are disabling the babel cache in nyc this might work. Just not sure how we plan to implement the --all processing when the babel plugin is used.

alexjeffburke · 2016-07-11T22:56:30Z

Hey, if this has been previously mentioned apologies - I had a project recently where I wanted to use nyc but ended up at the older istanbul bin primary for the ability to run coverage to a JSON file and then run istanbul report after the fact. This was for a node lib that had been packaged for the browser with browserify - I found that command pair immensely useful to get browser coverage over the library. I think I also used the browserify-istanbul transform. Just mentioning in case you needed someone to speak up for the use case. And lastly, hank you ever so much for tools. AJB

This was referenced Jun 27, 2016

Implement --all without requiring modules #183

Closed

Update istanbul dependency per https://nodesecurity.io/advisories/118 #284

Closed

work with Istanbul, knock ES2015 support out of the park #282

Closed

jamestalmage mentioned this pull request Jun 27, 2016

Move to istanbul organization? istanbuljs/append-transform#8

Closed

kentcdodds reviewed Jun 27, 2016
View reviewed changes

bcoe mentioned this pull request Jun 28, 2016

fix: nyc@7 places files in node_modules/.cache/* that should be excluded npm/npm#13201

Closed

JaKXz mentioned this pull request Jun 28, 2016

[email protected] lcov report broken #288

Closed

bcoe mentioned this pull request Jul 2, 2016

combine functionality of append-transform and istanbul-lib-hook #292

Closed

Benjamin Coe added 2 commits July 2, 2016 14:42

feat: switching over to new istanbul API

e746db7

fix: moved to using approach that @gotwarlost suggested, rather than …

fb5f91b

…relying on the preamble

Benjamin Coe added 7 commits July 2, 2016 14:42

feat: reworking nyc to become the official bin for the new Istanbul CLI.

b4160e8

docs: merge README.md changes

acececd

chore: roll back to 6.6.1 in package.json, we'll use standard-version…

d096880

… for the actual bump

fix: loosen babylon settings, based on testing against ava

82568d7

chore: add test for native es6 instrumentation

6b03014

fix: a bit contrived, but updated to ES6 syntax that was handled poor…

16a7405

…ly by old Istanbul

feat: switch to using istanbul-lib-source-maps

2f1af27

bcoe force-pushed the nyc-7 branch from 48929e6 to 2f1af27 Compare July 2, 2016 21:43

fix: update bundle

74b1487

bcoe mentioned this pull request Jul 4, 2016

7.0.0-alpha4 + babel-plugin-istanbul breaks transpilation for test runner #294

Closed

bcoe merged commit 61a05ea into master Jul 4, 2016

bcoe deleted the nyc-7 branch July 4, 2016 06:42

github-actions bot mentioned this pull request Apr 19, 2024

chore(main): release nyc 17.0.0 #1558

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: nyc is being refactored to become the official Istanbul 1.0 bin #286

feat: nyc is being refactored to become the official Istanbul 1.0 bin #286

bcoe commented Jun 26, 2016 •

edited

Loading

bcoe commented Jun 27, 2016 •

edited

Loading

bcoe commented Jun 27, 2016

bcoe commented Jun 27, 2016

jamestalmage commented Jun 27, 2016

bcoe commented Jun 27, 2016

kentcdodds Jun 27, 2016

bcoe Jun 27, 2016

kentcdodds Jun 27, 2016

bcoe commented Jun 27, 2016

JaKXz commented Jun 27, 2016 •

edited

Loading

gotwarlost commented Jun 27, 2016

gotwarlost commented Jun 27, 2016

gotwarlost commented Jun 27, 2016

gotwarlost commented Jun 27, 2016

bcoe commented Jun 27, 2016

addaleax commented Jun 27, 2016

bcoe commented Jun 28, 2016

kentcdodds commented Jun 28, 2016

bcoe commented Jun 29, 2016

kentcdodds commented Jun 29, 2016

gotwarlost commented Jun 29, 2016

bcoe commented Jun 29, 2016

gotwarlost commented Jun 29, 2016

bcoe commented Jul 2, 2016 •

edited

Loading

gotwarlost commented Jul 3, 2016

jamestalmage commented Jul 6, 2016

gotwarlost commented Jul 10, 2016

gotwarlost commented Jul 10, 2016 •

edited

Loading

alexjeffburke commented Jul 11, 2016

feat: nyc is being refactored to become the official Istanbul 1.0 bin #286

feat: nyc is being refactored to become the official Istanbul 1.0 bin #286

Conversation

bcoe commented Jun 26, 2016 • edited Loading

Features of Note

Breaking Changes

Todo

bcoe commented Jun 27, 2016 • edited Loading

bcoe commented Jun 27, 2016

package size update

testing update

bcoe commented Jun 27, 2016

jamestalmage commented Jun 27, 2016

bcoe commented Jun 27, 2016

kentcdodds Jun 27, 2016

Choose a reason for hiding this comment

bcoe Jun 27, 2016

Choose a reason for hiding this comment

kentcdodds Jun 27, 2016

Choose a reason for hiding this comment

bcoe commented Jun 27, 2016

JaKXz commented Jun 27, 2016 • edited Loading

gotwarlost commented Jun 27, 2016

gotwarlost commented Jun 27, 2016

gotwarlost commented Jun 27, 2016

gotwarlost commented Jun 27, 2016

bcoe commented Jun 27, 2016

addaleax commented Jun 27, 2016

bcoe commented Jun 28, 2016

kentcdodds commented Jun 28, 2016

bcoe commented Jun 29, 2016

kentcdodds commented Jun 29, 2016

gotwarlost commented Jun 29, 2016

bcoe commented Jun 29, 2016

gotwarlost commented Jun 29, 2016

bcoe commented Jul 2, 2016 • edited Loading

gotwarlost commented Jul 3, 2016

jamestalmage commented Jul 6, 2016

gotwarlost commented Jul 10, 2016

gotwarlost commented Jul 10, 2016 • edited Loading

alexjeffburke commented Jul 11, 2016

bcoe commented Jun 26, 2016 •

edited

Loading

bcoe commented Jun 27, 2016 •

edited

Loading

JaKXz commented Jun 27, 2016 •

edited

Loading

bcoe commented Jul 2, 2016 •

edited

Loading

gotwarlost commented Jul 10, 2016 •

edited

Loading