allow libraries that override require to instrument babel code - #3062

I like the idea. Basically allowing chains of extension transforms by preserving the last. Kinda like calling super in a method.

👍

Current coverage is 89.23%

Merging #3062 into master will increase coverage by +0.17% as of 2ea9b16

@@            master   #3062   diff @@
======================================
  Files          214     214       
  Stmts        15468   15471     +3
  Branches         0       0       
  Methods          0       0       
======================================
+ Hit          13777   13806    +29
  Partial          0       0       
+ Missed        1691    1665    -26

Review entire Coverage Diff as of 2ea9b16

Powered by Codecov. Updated on successful CI builds.

Happy to start to add some tests for babel-register if there's buy in for this feature 👍

I don't know if it makes sense to add this into Babel. IMO there should be proper node APIs for extension overloading and interception such as this.

@sebmck I agree that it would be awesome if Node's require API had a mechanism for creating a stack of require transformations -- I'm happy to start a discussion on Node core around this topic.

Having said this, it would be awesome if we could get some sort of hook in place in the interim. babel-register is a great approach for writing tests that take advantage of ES6 features, but with the current implementation I see no easy way to use it in conjunction with a tool like Istanbul; this significantly detracts from its usefulness.

@sebmck @bcoe I like the idea of handling this at the babel level as require.extensions is officially deprecated and changes are very unlikely to be supported or implemented.

I would prefer to see a different signature however. In this case, we're only allowing one additional hook specifically for instrumentation. It would be nice to see an api that would allow an arbitrary number of hooks like the presets and plugins options.

@sebmck should I rebase this? I'm open to alternative suggestions as to how we could add Istanbul/nyc support to babel's instrumentation hooks -- it would be pretty slick to be able to have both es6 tests and coverage, in a sanctioned, easy-to-document, way.

I'm just passing by to appreciate the work all of you has been doing. I'm researching for some good time how to get istanbul working with Babel 6 and landed here. So, thanks and good luck. 😄
If this feature doesn't land, I would totally appreciate directions on how to cover my code when using Babel >= 6

@bcoe when you rebase it, maybe it's a good idea to fixup the var -> let commit... Thanks again

not pretty, but chatting with @isaacs today, came up with an approach for keeping code-coverage instrumentation without exposing any hooks in babel-register:

NYC.prototype._wrapRequire = function () {
  var _this = this

  var cache = {}
  var externalRrequireHook = null
  var requireHook = function (module, filename) {
    // break cyclical behavior between the
    // two require hooks.
    if (cache[filename]) return
    cache[filename] = true

    // allow the last require hook registered
    // to perform the compile step.
    var content = null
    if (externalRrequireHook) {
      externalRrequireHook({
        _compile: function (compiledSrc) {
          content = compiledSrc
        }
      }, filename)
    }

    // now instrument the compiled code.
    var obj = null
    if (content) obj = _this.addContent(filename, content)
    else obj = _this.addFile(filename, false)

    module._compile(obj.content, filename)
  }

  // use a getter and setter to capture any external
  // require hooks that are registered, e.g., babel-core/register
  require.extensions.__defineGetter__('.js', function () {
    return requireHook
  })

  require.extensions.__defineSetter__('.js', function (value) {
    externalRrequireHook = value
  })
}

@bcoe: Why was this closed? This looks really useful, currently istanbul is broken for me.

Sorry, @sebmck, see my above comment I forgot to mention you. I was wondering why this was closed, it seems very useful.

@bcoe your example has basically the same issue as the one this PR aims to fix (except it has been kicked to the require.extensions global. By not having a chain, only one lib can take this approach without collisions. The current PR does not suffer from that same issue, but essentially just "calling super".

@bcoe, @sebmck : since node is unlikely to introduce an api for this, would you prefer if there was some agreed apon library/standard for doing these things? If so (not that I'm in any way qualified to do so, because I'm not), I'm happy to coordinate a standard/write a library to do so. (Actually, now that I think about it, it might be possible to write something that magically makes this work without involvement from any of the involved libraries (using setters), but that would be incredibly hacky, so...)

would you prefer if there was some agreed apon library/standard for doing these things

👍

I think a library is a better choice. A standard requires people read it, follow it correctly, and update their implementations as the standard evolves. An agreed upon library can mitigate a lot of that.

Ultimately, we settled on this approach in nyc:

https://github.com/bcoe/nyc/blob/master/index.js#L130

which works great with babel.

I agree with @ariporad, this logic would be a great candidate for moving into a module that could be consumed by other libraries that munge the extension hook.

Ok, let's move this discussion (for the most part) to istanbuljs/nyc#70.

would you prefer if there was some agreed apon library/standard for doing these things

👍 this sounds like the best path forward. It is reasonable to request similar libraries use some agreed upon coordinator. That coordinator can use various approaches to fail fast (and in an informing way) if something is sideways.

Ok everyone, I'm going to start working on this module, you can keep track of it in ariporad/pirates. Also, I created danez/pirates#1, so shall we move discussion there to stop filling up the babel issue tracker?

Ok everyone (I mentioned this in the other thread too, but I'm not sure if @sebmck is on that one), I got pirates mostly done! It's now available on npm, and I've opened pull requests for babel (#3139) and istanbul (istanbuljs-archived-repos/istanbul-lib-hook#5), and a PR for nyc is in the works. If you have any other modules you like that use require hooks, convince them to adopt pirates! It's super easy and reduces complexity!

After looking into having getters and setters for dealing with naughty require hooks, I ended up dropping that idea (for now), because basically any hook that misbehaves is using Module.prototype._compile instead of module._compile, which I haven't been able to figure out how to fix.

Enjoy!