webpack-contrib / istanbul-instrumenter-loader

Istanbul Instrumenter Loader
MIT License
273 stars 65 forks source link

Sourcemaps not tracing back to the source #73

Open filipesilva opened 6 years ago

filipesilva commented 6 years ago

The sourcemaps that istanbul-instrumenter-loader returns are not using the input sourcemap to provide sourcemaps back to the source.

This can be reproduced in the tests for this repo.

var _createClass = function () { function defineProperties(target, props) { for (var i = 0; i < props.length; i++) { var descriptor = props[i]; descriptor.enumerable = descriptor.enumerable || false; descriptor.configurable = true; if ("value" in descriptor) descriptor.writable = true; Object.defineProperty(target, descriptor.key, descriptor); } } return function (Constructor, protoProps, staticProps) { if (protoProps) defineProperties(Constructor.prototype, protoProps); if (staticProps) defineProperties(Constructor, staticProps); return Constructor; }; }();

function _classCallCheck(instance, Constructor) { if (!(instance instanceof Constructor)) { throw new TypeError("Cannot call a class as a function"); } }

module.exports = function () { function Foo() { _classCallCheck(this, Foo); }

_createClass(Foo, [{ key: "bar", value: function bar() { return !!this; } }]);

return Foo; }();

// WEBPACK FOOTER // // /fixtures/basic.js

- which is quite different from the original source (`tests/fixtures/basic.js`):

module.exports = class Foo { bar() { return !!this; } };



I tried tracing this around for a bit and couldn't find where it goes wrong, but it seems that the instrumenter is receiving the input sourcemap but `instrumenter.lastSourceMap()` isn't a sourcemap back to the source.
calebeno commented 6 years ago

Getting impacted by this issue currently. Any timeline on a possible fix?

felixfbecker commented 6 years ago

This is preventing me from getting proper coverage output in https://github.com/unional/domture/issues/9

Predhin commented 6 years ago

Any updates on this issue?

Thanks.

csentis commented 6 years ago

@d3viant0ne Hi Joshua, Being a fan of your thoughtful work, kindly let me encourage you to re-evaluate this issue. From my point of view it is less an inconvenience and more a reduction of this good middleware‘s use to the lowest possible grade - at least in Angular-CLI-instrumented TS projects. Is it possible for you to give this a push? Thanks greatly for reconsidering!

Edit: I greatly overexaggerated. It is more like turning a sniper rifle into a shotgun.

escalonn commented 5 years ago

Any updates? This blocks coverage in SonarQube for Angular with the normal setup, since SonarQube needs the generated LCOV file's line numbers to match the TS source files.

adamburgess commented 5 years ago

Here's an attempt at fixing it: https://github.com/adamburgess/istanbul-instrumenter-loader/commit/70355d4a578f330b7e24b207b4573ddd9f2a9388#diff-1fdf421c05c1140f6d71444ea2b27638 (lots of changes in this diff because I upgraded webpack/dependencies which were quite old, but the actual changes linked are very small) it's on npm if you want a quick install, @adamburgess/istanbul-instrumenter-loader

istanbul-instrumenter-lib doesn't use the sourcemap as an input, it just adds it to the output file. but you can use merge-source-map (which internally uses source-map) to merge the output source map with the input one, which seems to work.

In my projects, stack traces now have correct line numbers.

felixfbecker commented 5 years ago

Are you planning to open a PR for that?

adamburgess commented 5 years ago

it's been almost a year since a functional change landed in this repo, so no, I'll just leave it as a separate repo. anyone else can take the changes and do what they want with them, of course.

felixfbecker commented 5 years ago

Some people would still appreciate a PR for visibility. Some community members might be able to give feedback on it. If there are enough PRs that noone is merging we should join efforts on one fork. Or just get the repo more people with write access.

filipesilva commented 5 years ago

Added a PR based on the work by @adamburgess at #73 (comment). @michael-ciniawsky @d3viant0ne WDYT of this fix?

JamesMessinger commented 5 years ago

I created a new npm package named coverage-istanbul-loader. It includes the fix mentioned above as well as other fixes and updated dependencies.