olov / ng-annotate

Add, remove and rebuild AngularJS dependency injection annotations
MIT License
2.03k stars 150 forks source link

Fix inconsistency between `map` and `sourcemap` #165

Closed smrq closed 9 years ago

mgol commented 9 years ago

It still doesn't work for me. Could you checkout my grunt-ng-annotate's WIP branch combine-sourcemap, run npm install, swap ng-annotate in node_modules to your version (npm link ng-annotate or sth) and then npm test? You'll see that:

  1. Both non-combined map tests fail, only the combined one passes.
  2. If you revert just the changes to tasks/ng-annotate.js from the last commit on the combine-sourcemap branch (it's just sourcemap -> map renaming) the combined tests fail & the individual source map ones succeed.

Let me know if you need help with getting it running.

mgol commented 9 years ago

Ahh, I'm sorry, I tested on the wrong branch. <badum, psst> It seems to work fine. :) Tests for this whole behavior would be useful (I've just added them to grunt-ng-annotate but it seems they should be here) but it's better to have it working than broken anyway.

@olov could you merge?

omsmith commented 9 years ago

Great, I'll have to investigate and see if this clears up https://github.com/substack/node-browserify/issues/1192

mgol commented 9 years ago

Ah, one thing - the generated path seems wrong unless I broke sth. You can try on my combine-sourcemap branch I mentioned before. I'm transforming test/fixtures/not-annotated-es6.js via babel to test/tmp/not-annotated-es6.js and then pass it via grunt-ng-annotate & generate test/tmp/not-annotated-es6-source-map.js. The map file's sources is ['not-annotated-es6.js'] so it points to the second file instead of the first.

EDIT: My first source map (the one generated by babel) looks like:

{ version: 3,
  sources: [ 'test/fixtures/not-annotated-es6.js' ],
  names: [],
  mappings: ';;AAAA,CAAC,...etc.',
  file: 'test/fixtures/not-annotated-es6.js',
  sourceRoot: '/Users/mgol/Documents/projects/public/mine/grunt-ng-annotate',
  sourcesContent: ['The original code']
}

The second one (generated by ng-annotate when passed on the babel output) is:

{ version: 3,
  sources: [ 'not-annotated-es6.js' ],
  names: [],
  mappings: 'AAAA;;AAEA,CAAC,...etc.',
  sourcesContent: ['The transformed code']
}

I'd expect the full path here: ['/Users/mgol/Documents/projects/public/mine/grunt-ng-annotate/test/fixtures/not-annotated-es6.js']. Also, sourcesContent is wrong; it seems combining doesn't happen.

So this PR seems to fix the non-combining case but breaks the combining one.

This could really use some tests... See what I wrote for grunt-ng-annotate: https://github.com/mzgol/grunt-ng-annotate/blob/aa0efb91dbe57862357c51128cb24937cd5d46e7/test/spec/api.js#L79-L137

smrq commented 9 years ago

Are you totally sure your test is correct? Did the parameter you are passing in as inFile exactly match babel's output? That's how the sourcemap combination algorithm works (for more info than you can shake a stick at, see: mozilla/source-map).

Because of the possibility of misconfiguration, I just updated the code to ignore inFile when there is an incoming sourcemap with the file property specified. That should wire up the combination step correctly at all times.

smrq commented 9 years ago

I've also done the same for sourceRoot, for the same reason.

mgol commented 9 years ago

Are you totally sure your test is correct? Did the parameter you are passing in as inFile exactly match babel's output in sources?

It didn't. babel returns the map with sources: [ 'test/fixtures/not-annotated-es6.js' ]; this was relative to the main project directory so I added sourceRoot: '/Users/mgol/Documents/projects/public/mine/grunt-ng-annotate'. The inFile that I'm passing is just not-annotated-es6.js since both source & destination files are in the same directory.

I'm not sure how to do it all differently without reading the incoming source map if it exists. IMO it shouldn't matter - I'm just providing an input file and the source map generating algorithm should itself look further.

In any case, your newest version of the patch passes my tests so it should be OK. :+1:

smrq commented 9 years ago

Awesome, glad it works for you now! The internals of sourcemaps are just so obnoxiously fiddly.

@olov - Should be good to merge, I think.