fix: pass correct args to tranformer (#153)

[kevinkir]

No description provided.

[coveralls]

Coverage remained the same at 88.028% when pulling 8692388 on kevinkir:master into c13d273 on istanbuljs:master.

[CodeTroopers]

Before apply this PR, have a look to this issue #89 and this merged PR #99

[kevinkir]

@CodeTroopers thanks for pointing that out. This complicates things a bit. It seems like the problem is that the expected signature of transformer was changed for hookRunInThisContext, but not for hookRequire or hookCreateScript.

Did the transformer signature actually need to be changed to fix #89 ? I don't see why it needed to be changed, but I could be missing something.

I think that it would be least confusing if "transformer" meant the same thing everywhere. Either it should always accept an options object or it should always accept a filename. I can update my changes to also update the documentation for hookRunInThisContext's transformer to {Function(code, filePath)}.

It might be annoying to people who have adopted the new "transformer" signature to have to flip-flop back to the old way, and it's kind of a sneaky breaking change, but it still makes the most sense to me, and I think it's the least confusing long-term. What do you think?

Updating the documentation of hookRequire and hookCreateScript to reflect the new transformer signature would also be a possibility. However, the options object with a "filename" property is an implementation detail of runInThisContext, so it would seem strange to me to use it in the unrelated context of hookRequire and hookCreateScript.

[CodeTroopers]

Well I always remain perplex on the role and the implementation of this transformer function. Like i asked in the #99 PR:

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

I'm not sure to understand all the behavior requested by hooking mechanisms but it seems that there is a redundancy effect by offering a hook of require and runInThisContext whereas require goes through runInThisContext

[kevinkir]

@CodeTroopers I believe @bcoe provided some explanation for the advantages of hooking require over hooking runInThisContext in the #99 PR:

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).

From my perspective, I personally don't know the inner workings of require, and I'm sure a lot of other people don't either. However, hookRequire has a very clear purpose to me, even though I don't know how require works. I would not guess that hooking require would happen as a side-effect of runInThisContext.

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

This concern does not make sense to me. The type and count of arguments for the functions that you are hooking seems like an implementation detail of those functions. I don't see what it has to do with the user-supplied transformation callback. The transform callback has a very simple purpose. It takes the original code and transforms it in some way that the user wants. The way I see it, passing the filepath to the transformation function is just a convenience. If you want to apply different transformation logic for different sets of files, you could achieve that by calling hookRequire multiple times. If you want to keep track of which files have been transformed, you can do that with options.postLoadHook.

One important thing to consider, however, is that it is potentially nice for different parts of the istanbul API to work well together. In the past, Instrumener.instrumentSync() could be used to create a transformer function for hookRequire. However, as it is now, you have to do some nonsense like

hookRequire('src/**/*', (code, { filename }) => instrumenter.instrumentSync(filename));

as opposed to

hookRequire('src/**/*', instrumenter.instrumentSync.bind(instrumenter));

(see https://github.com/istanbuljs/istanbuljs/blob/master/packages/istanbul-lib-instrument/src/instrumenter.js#L77 for instrumentSync implementation)

It is really just a coincidence that runInThisContext happens to also use the filepath. Even if it didn't, the filepath could still be potentially useful for your transformer function.

[CodeTroopers]

Ok, thanks for your explanations.
I not aggree with you on this point :

The type and count of arguments for the functions that you are hooking seems like an implementation detail of those functions. I don't see what it has to do with the user-supplied transformation callback.

When i hook a function, I want to be able in my transformer function to access all the arguments passed to the original function and not only code and filename. So to have a unique call point for all transformer functions leave me puzzled.

[bcoe]

@kevinkir @CodeTroopers I don't care too much about the changing function signature (going from a string to an object). What concerns me as a maintainer of this library, is that we should have flagged this change as a breaking change, this was the point I was trying to make in #99 we need to take breaking API changes seriously, so that we're bumping major semver versions rather than minor.

I think the right thing to do is to land @kevinkir's changes, since we didn't announce the API signature change as a breaking change; and this version of Istanbul has only been out for a few weeks.