andrey-skl / ng-annotate-loader

Webpack loader to annotate angular applications
MIT License
120 stars 28 forks source link

Incorrect chained source maps #10

Closed jvmccarthy closed 9 years ago

jvmccarthy commented 9 years ago

First, thank you for creating this much-needed loader.

I'm having an issue right now where source map line numbers aren't lining up after running the ng-annotate-loader. I've been having difficulty debugging because placing breakpoints in the Chrome debugger don't land on the correct line. Because the line numbers are off, stepping in the debugger isn't working either.

Just looking into source maps is difficult for that matter. I found that by opening my generated webpack bundle, I can click to place a breakpoint and Chrome will take me to the mapped line in the source map. Using this technique, I can tell the source map becomes incorrect after the first ng-annotate addition (after a line containing $inject). I've also tested removing the ng-annotate-loader, and then I get correct source mappings. The lines appear shifted for each line ng-annotate adds, as if the source map isn't being altered at all by ng-annotate.

I'm using ng-annotate-loader chained with the babel-loader like this,

{ test: /\.js$/, loader: 'ng-annotate!babel' },

with devtool: 'source-map'

I can appreciate that ng-annotate-loader is the middleman here and that this may be an ng-annotate issue. I've tried a number of different things including rolling back to ng-annotate-loader@0.0.4 or rolling to ng-annotate@1.0.2 and source-map@0.4.4 (the current latest versions). No combination seems to work. I've also tried debugging a bit in loader.js, but I get lost trying to compare the merged map from the original.

Any suggestions would be appreciated. Is there a quick way I can debug if ng-annotate isn't generating a proper source map? The plumbing in ng-annotate-loader looks right, but I'm not sure. I thought I would start here first.

Thanks!

jvmccarthy commented 9 years ago

Using sokra's source-map visualization, I'm seeing correct output from ng-annotate. That is, the produced source and source map match up, before and after lines added with $inject.

Once the source map gets merged with the incoming map, the symbols don't match up correctly. It looks like something is going wrong in the merge. I'm comparing this with gulp-ng-annotate which uses vinyl-sourcemaps-apply. It looks like they're applying these maps in the opposite order, starting from the map ng-annotate produces and then applying the existing source map. I tried changing the order similarly, but it's still not working (I'm getting the opposite effect, moving lines up instead of down). I'm reading through the source-map API but not getting anywhere fast.

andrey-skl commented 9 years ago

@jvmccarthy Thank you for reporting! I can confirm that issue is exist, and I see it from time to time, but I still have no idea how to fix it. Any help is welcome.

andrey-skl commented 9 years ago

As a temporary solution I can suggest turning off ng-annotate-loader while developing..

jvmccarthy commented 9 years ago

Thanks for the quick responses. I use ng-strict-di to help detect missing annotations during development. I'd rather not disable that.

I've been trying to follow different examples on how to properly merge source maps. I've got a fork that tries to follow BabelJS's example: https://github.com/jvmccarthy/ng-annotate-loader/tree/sourcemaps. In my testing, the line numbers are still off by a single line. See also the babel example added on that branch.

andrey-skl commented 9 years ago

@jvmccarthy I have checked that branch and sourcemaps there is matched correct as I see. Am I wrong? screen shot 2015-09-04 at 23 09 36

andrey-skl commented 9 years ago

@jvmccarthy please help me to find a reliable verification criterion

jvmccarthy commented 9 years ago

@huston007, you're right, the example is matching up with the source map visualizer. I can't figure out why in my application the line offset appears to be off by one (and is correct when I remove ng-annotate-loader). But, this appears to be better. I'll submit a PR.

andrey-skl commented 9 years ago

@jvmccarthy okay, thanks. I'm going to analyse it carefully.

andrey-skl commented 9 years ago

@jvmccarthy I have completely analysed the PR and fix some small issues. It works nice for me, and lines is completely matched now in my projects (was shifted before). Thanks a lot for amazing investigation! You've made debugging much easier for a lot of us.

Version 0.0.8 is published to NPM, please check it.

jvmccarthy commented 9 years ago

@huston007, thank you for working with me quickly on this. I'll give v0.0.8 a try. Thanks also for cleaning up the introduced issue.

jvmccarthy commented 9 years ago

@huston007, sorry to say, but the latest changes aren't producing correct source maps. I'm now getting the source produced by BabelJS instead of the original source. I'll see if I can look into this some more...

andrey-skl commented 9 years ago

@jvmccarthy sorry, have missed that - they looked similar. Looks like issue still exist

andrey-skl commented 9 years ago

@jvmccarthy have reverted one of that change (with sources[0] file). Now it works on example, please check it. (npm version 0.0.9-beta2)

jvmccarthy commented 9 years ago

@huston007, thanks, that's looking better.

I also looked at the source-maps API again and took another swing at this using the loader context resourcePath for the source file name. Honestly, I'm not sure how it was working before with the full request being returned from loader-utils getCurrentRequest (perhaps something is splitting the request?). I pulled out the logic to merge the source maps to its own function. I think the code is cleaner now. It's on my sourcemaps branch. Should I submit another PR for this, or do we want to call it good with your latest changes?

Thanks again :)

andrey-skl commented 9 years ago

@jvmccarthy looks like that filename variable have no effect no more. I tried to write var filename = "" and it still works properly (tests found no changes).

But please send a PR because you have extracted sourcemap generation function, it looks better)