Use source maps from coverage map

[MichaReiser]

Part of istanbuljs-archived-repos/istanbul-lib-instrument#23

Alternative could be to move it to remap-istanbul instead and implement there a custom source
map resolution strategy

[yahoocla]

CLA is valid!

[bcoe]

@DatenMetzgerX nyc should already be loading the source-map if it's included in the footer of an instrumented file:

NYC.prototype._handleSourceMap = function (cacheDir, code, hash, filename) {
  var sourceMap = convertSourceMap.fromSource(code) || convertSourceMap.fromMapFileSource(code, path.dirname(filename))
  if (sourceMap) {
    if (hash) {
      var mapPath = path.join(cacheDir, hash + '.map')
      fs.writeFileSync(mapPath, sourceMap.toJSON())
    } else {
      this.sourceMapCache.registerMap(filename, sourceMap.sourcemap)
    }
  }
}

Is there a chance we could piggyback off of this, rather than adding a new approach for passing a source-map around? I think I'm not quite following the scenario we're addressing.

[MichaReiser]

Hy bcoe.

I understand that this solution looks quite strange and the reason for it might not be visible right away (needed myself two days to figure out that the other approaches are probably not working).

Therefore, we might need a more detailed use case.

We have two typescript files

file world.ts

export class World {
  sayHy() { console.log("Hello world");
}

file: main.ts

import { World } from "./world";

const world = new World();
world.sayHy();

As this program should be used in the browser, a bundler like Rollup, webpack or browserify is used to transpile and bundle all the files together into lib.js. The output might look like

lib.js

class World {
  sayHy() { console.log("Hello world");
}

const world = new World();
world.sayHy();

//# sourceMappingURL=....

The build chain, including instrumentation then is:

source (0) -> (1) typescript -> (2) instrumenter -> (3) bundling

whereas the step 1 and to are performed on a per file basis.

Potentially additional steps, like transpiling ES6 to ES5 using babel, could be performed before or after step 2.

The point now is, the instrumenter (2) creates the line numbers based on the output of typescript (1). Therefore, if the code coverage should be mapped to the original code, the source map generated by typescript must be used (from 1 to 0).

But the source map generated in step 1 is not added into the lib.js as one further additional step is performed. All files are crunched together and therefore, an additional sourcemap containing the mapping from step 3 to 0, is included and added to lib.js (potentially inline). This source map cannot be used to remap the code coverage, as the line numbers used by the instrumenter do not match the line numbers in the bundled lib.js (potentially minified).

Your solution of adding an inline source map in step 2 might work, but I think this is no longer compliant with the source map specification. In this case, the lib.js file would contain multiple source maps (one containing the mapping from step 3 to 0, and one for each file containing the mapping from 1 back to 0).

class World {
  sayHy() { console.log("Hello world");
}
//# sourceMappingURL=.... world.ts: 1 to 0

const world = new World();
world.sayHy();
//# sourceMappingURL=.... main.ts: 1 to 0

//# sourceMappingURL=.... mapping from 3 to 0

Even if this is standard compliant, I don't think that the browsers or other unit testing tools would pick that up right and are therefore no longer capable to map error messages back to the original source, something that is quite helpful with failing tests. Therefore, I looked for an alternative solution. And from my point of view this is the only possible solution as the source map from step 1 to 0 is otherwise no longer available in the output and therefore must be saved and is part of the instrumentation.

Instrumenting the bundled code is also no alternative. In this case, the unit tests and all used libraries are instrumented too. This can break unit tests - e.g. if depending on function names - or libraries.

So I hope this explanation shows you why I didn't took the approach used by nyc (that is supported by istanbul out of the box). The way I integrated the approach is definitely discussable as I lack a deep understanding for the source map part of istanbul

[bcoe]

@DatenMetzgerX I think it would be worth syncing up with @kpdecker on this issue too:

istanbuljs/nyc#393

We have source-map functionality implemented at several layers in istanbul, which as you outline well, gets complicated when you start applying a multi-step build process.

My main ask, is that we figure out a way to make this work as if by magic, in as many scenarios as possible.

[MichaReiser]

@bcoe
I think this is actually not the same issue. The pull request from @kpdecker generates a source map between pre and post instrumentation (step 2). This is already supported by istanbul and was just missing in nyc.

And by the way, I'm looking for an nyc independent option as using nyc is not an option for me. So i think it should be supported by istanbul and any tools using it can support it (where appropriate)... mostly only needed if bundlers are used.

[bcoe]

@kpdecker @novemberborn @JaKXz any thoughts on this pull request and API; I'd like a second opinion since I don't personally use source-maps in my build-chain:

• what do folks think of this API.
• would you use it?

@DatenMetzgerX sorry for the slow turn around; hit a bit of a log jam with regards to my OSS work recently. I think I better understand where you're coming from with this feature now. A couple follow up questions.

• for your nyc independent approach to instrumentation, I'm curious what collection of Istanbul libraries do you use for test-coverage.
• could you provide some sample code, with regards to how you would use this new API; I'd like to add sections to the pertinent documentation to describe this feature in detail.

[MichaReiser]

No problem. I'm just hoping to be able to switch back to the official version sometime soon in the future 😉

About nyc. I like the tool and I'm using it in two of my other (node) projects where no bundling is needed. But that's not the case for my other project that runs in the browser. But maybe it is possible to use nyc with webpack, browserify, rollup or other bundlers...?

The setup is visible in my latest project. I'm using WebPack for bundling the files together into a single output.

• For the instrumentation, the latest version of istanbul-instrumenter-loader that is using istanbul-lib-instrument.
• The tests are executed using karma with karma-coverage (that is still using the old istanbul version...). The old version outputs only a json report, as html reports fail to resolve the source code if the code is instrumented with istanbul-lib-instrument (the option for including the source is no longer available)
• I'm then using remap-istanbul to map the coverage information back to the original source files (using the suggested extension). I would use istanbul-lib-source-map if possible, but as mentioned, karma-coverage is still stuck with old istanbul....

To the second question. The API is used over istanbuljs-archived-repos/istanbul-lib-instrument#23, as you can see in the following pull request istanbul-instrumenter-loader#29 (that is waiting that this change is accepted 😉). Or do you need more detail?

I may help you with documenting the new feature if it is accepted and we have elaborated how it looks in the end.

[deepsweet]

another real life example in which I need to pass an original source maps from instrumenter to reporter step – start-istanbul. As you can see, I was able to manage it with a shared "remapper" singleton, but I'm not so happy with this way. Also, this approach is impossible in istanbul-instrumenter-loader, because we don't have a full control over the reporter step. So... IMO storing the original source map in a coverage object and then getting it from there is a good idea for programmatic usage of Istanbul.

[bcoe]

@mourner would love your opinion on this API, since you've been digging through the instrumenter and sourcemap libraries quite a bit.

[mourner]

@bcoe unfortunately I don't have enough knowledge to comment on this particular change.

[novemberborn]

any thoughts on this pull request and API; I'd like a second opinion since I don't personally use source-maps in my build-chain:

what do folks think of this API.
would you use it?

I understand the need. We may need something like this when we add browser support in AVA.

I'm not familiar with the various Istanbul libraries so I can't comment specifically on the changes.

[MichaReiser]

Any updates on this topic?

[bcoe]

@DatenMetzgerX I'm going to defer to your expertise on this topic, and land this.

[bcoe]

sorry for the slow turnaround on this! let's get this landed this week.

I think the API looks fine, let's work on adding some tests to both this and istanbul-lib-instrument 👍

[bcoe]

@DatenMetzgerX sorry for taking so long to land this, I was hoping someone would chime-in to help with the review who has a more source-map-focused workflow.

Would it be possible to add a test for this new logic that fails until we land the changes in istanbul-lib-instrument.

[MichaReiser]

I added the test but it actually does not fail... it does not depend on istanbul-lib-instrument. I hope thats fine...

[MichaReiser]

Rebased and added tests