Fix hook runInThisContext

[CodeTroopers]

Fix #89
Hook runInThisContext with options as parameter instead of filename
Hook on require and hook on runInThisContext can not be activated at the same time because require call runInThisContext

[CodeTroopers]

Build is Ok with NodeJs 6 but a unit test fail in Node JS 4

[bcoe]

@CodeTroopers it looks like you and @pangrr are working towards the same goal? (hooking vm.runInContext).

I'm leaning towards @pangrr's implementation; I think that adding this as a new method (rather than modifying old methods) is better, as it provides more backwards compatibility. Perhaps you could pitch in and help test @pangrr's pull so we can see it over the finish line?

Or perhaps I'm missing a nuance around the work you're both doing?

[CodeTroopers]

I will agree with you if it was the same function. But @pangrr try to hook runInContext to run code in a special context when i try to hook runInThisContext to run code in the current context.

And my modification is in theory very basic : replace the hook of runInthisContext(code[, **filename**]) by a hook of runInthisContext(code[, **options**]).
I'm very surprised that istanbuljs hook runInthisContext(code[, **filename**]) although this exists in NodeJS v0.10.48 but it becomes runInthisContext(code[, **options**]) since NodeJS v0.12.0

[bcoe]

@pangrr mind helping out with the code review, I haven't used Node's vm functionality in a few years and want to make sure that we don't land something that breaks things for anyone.

@CodeTroopers @pangrr my biggest immediate question is why we're retiring the transformer API method; this would definitely need to be a major change ... could we instead make our work additive?

[CodeTroopers]

Ok, I didn't see that transformer was exposed by the api (For now, I don't understand the utility of transformer but I have only a limited overview of the library)
I can reintegrate transformer but this will have no effect on the build problem with NodeJS 4

[pangrr]

The failing test happened at this assert: coverageMap[path.resolve(codeRoot, 'context.js')] is undefined.
Some console.log may help debug.

[bcoe]

@CodeTroopers @pangrr could we work on getting this over the finish line? I'm not experienced with the runInThisContext API, so I don't feel comfortable landing this until we figure out some of the issues with tests -- also I believe we need a rebase with @pangrr's work around runInContext.

[CodeTroopers]

@bcoe could you explain us wat is the purpose of the transformer function in particularity why use a transformer function (as argument of transformFn in hook.js) to hook several functions which have possibly different type and count of arguments.

Another thing, since require uses internally runInThisContext, the hook on require is unnecessary and the default behavior should only use a hook on runInThisContext and offers the possibility of hooking runInContext and runInNewContext by configuration

[SimonSimCity]

@CodeTroopers, @bcoe, @pangrr what's the progress of this PR and what's left to do? I need this for the referenced ticket in meteor-coverage ...

If you tell me what's missing in order to get this merged, I might be able to sit down and get into finding a good way of solving it.

[CodeTroopers]

@SimonSimCity Programmatically the PR work but unit test fails with NodeJs 4 and pass with NodeJs 6 due to internal change of require between this 2 versions.
On design the hook of require is not required and the hook of runInThisContext is sufficient because require uses internally runInThisContext but this can not be compatible with NodeJS4. And additionaly the hook of runInContext and runInNewContext could be added.

[bcoe]

@SimonSimCity @CodeTroopers I would love help seeing this over the finish line.

My main concern with the existing implementation is that it's a breaking API change; switching from an string representing a filename, to an options object. I think this could potentially cause issues for upstream libraries that were using the existing implementation of requireTransformer.

What I would love to see would be the addition of runInThisContextTransformer, in such a way that it doesn't change the API surface of requireTransformer.

[CodeTroopers]

@bcoe it's the case in the modification of 5 Oct
But all the hook mechanisms use the transformFn in hook.js, so my question of 17 Oct :

why use a transformer function to hook several functions which have possibly different type and count of arguments?

[bcoe]

On design the hook of require is not required and the hook of runInThisContext is sufficient because require uses internally runInThisContext but this can not be compatible with NodeJS4

The only advantage I see to hooking require is that it uses append-transform, which manages a stack of multiple transformations.

For nyc which is a command line application that integrates with much of the Istanbul API, we actually use caching-transform for hooking require, this has the advantage that it creates a cache of the instrumented files (significantly speeding up execution).

why use a transformer function to hook several functions which have possibly different type and count of arguments?

I don't quite follow this issue; haven't dove too deep into this part of the codebase -- is there a bug you see regarding argument count?

---

Mainly, I'd like to try to not change the function signature of anything; unless there's a compelling reason, e.g., we could rewrite thing something like this:

runInThisContextTransformer = function (code, options) {
   const filename = typeof options === 'string' ? options : options.filename
   return instrumenter.instrumentSync(code, filename);
};

and support the old API surface and the new one. I don't actually use istanbul-api myself, nyc uses the instrumenter, etc., directly. So, mainly just trying to keep the function signatures consistent so that we're not releasing a breaking change. Could we use a similar approach (sniffing out capabilities and argument types) to continue supporting Node 4?

[CodeTroopers]

is there a bug you see regarding argument count?

No bug, just a feeling that the transformer function can not be usefull because it is limited to the current prototype

Could we use a similar approach (sniffing out capabilities and argument types) to continue supporting Node 4?

No, this is not sufficient. Once again the main problem is that require use vm.runInThisContext since NodeJS 6. When we hook require we cannot hook runInThisContext in the same time because this create a code coverage instrumentation over a code coverage instrumentation. It's why i have removed the default hook on require when the hook on runInThisContext is used :

        if (config.hooks.hookRunInThisContext()) {
-            hook.hookRunInThisContext(matchFn, transformer, hookOpts);
+            hook.hookRunInThisContext(matchFn, runInThisContextTransformer, hookOpts);
+        } else {
+            disabler = hook.hookRequire(matchFn, requireTransformer, hookOpts);
         }
-        disabler = hook.hookRequire(matchFn, requireTransformer, hookOpts);

But with NodeJS 4, require don't use vm.runInThisContext, so the test for runInThisContext fails because code coverage instrumentation has not been applied on the require call because the default hook has been replaced by the hook of runInThisContext

[SimonSimCity]

@CodeTroopers Since this is a change in NodeJS 4 to 6, would it be an option to build in a condition for a specific version of NodeJS? As I understand this, it's a part that goes very much down into how NodeJS is built internally, right?

[CodeTroopers]

@SimonSimCity I added a test on the nodeJS version to add the hook on require when it is needed
@bcoe I added support for old api in transformer function

Now, all tests pass in my environment with NodeJS 4.8.4, NodeJS 6.8.1 and NodeJs 8.9.0.
It fails in AppVeyor with :

Cannot find module 'highlight.js'

Must I commit package-lock.json? i 'm not sure...

[SimonSimCity]

Hope you had a good time off keyboard by celebrating 🎅 and 🎆 - and I just wanted to bring this up on the table again ...

@bcoe @CodeTroopers I was able to make the package meteor-coverage compatible with the code of this PR. There was still one breaking change, which was mainly due to the fact, that the old version of this library only was compatible to the wrong way of calling vm.runInThisContext and it will only affect people who are using istanbul-lib-hook directly without istanbul-api. They will have to implement the changes you did to istanbul-api in their own code. See serut/meteor-coverage@49ae8d8 and 15e4e68 for more details.

The requirement of a change here though to me makes quite much sense, because the way of calling the method vm.runInThisContext changed, so I also expect the parameters of the hook to change. The only thing that might strike someone is, that even though you call it the old way (vm.runInThisContext(code, filename)) istanbul-lib-hook will convert it to the new parameters (vm.runInThisContext(code, { file: filename })). But making this backwards compatible would make the transformFn in istanbul-lib-hook just unnecessarily complicated and the one implementing the transformer function ins his own extension would also have to be made aware of that there once was an old way, which has dropped out of the documentation now.

Leaving the code like this is fine for me, because if you have done this wrong, you've probably for a long time not followed the changes on the NodeJS API and have to catch up - and your code anyways wasn't really doing what he was supposed to do 😅

[SimonSimCity]

@CodeTroopers @bcoe Could you please take a look at this again? I really need it merged ...

[bcoe]

@CodeTroopers mind rebasing with master; I've removed Node 4 from the appveyor rubric.

[CodeTroopers]

@bcoe I rebased with the current master. Like you have removed the NodeJS 4 appveyor build, Do you wish that I remove specific code for NodeJs 4 support? (NodeJS 4 end support is in April)

[SimonSimCity]

@bcoe Any chance we can get this through before the end of this month?

[serut]

Friendly ping