Empty mappings fix implemented. #251
Conversation
| util.assertEqualMaps(assert, map.toJSON(), util.testMapWithSourcesContent); | ||
| }; | ||
|
|
||
| exports['test .fromSourceMap with empty mappings'] = function (assert) { |
There was a problem hiding this comment.
Could you also add a test case here for the edge case I described above? (i.e. mappings are not empty, but only ever refer to one source).
ejpbruel
left a comment
ejpbruel
left a comment
There was a problem hiding this comment.
Hi dmurat!
Thanks for working on this. This indeed looks like an edge case we should fix.
Your PR looks good overall, but there are a few changes I would make before merging it. Could you look into that?
Sorry for the slow turnaround time on this PR. I promise to respond faster next time.
|
I updated pull request as I understood your request. I'm hoping that I understood it correctly :-) |
ejpbruel
left a comment
ejpbruel
left a comment
There was a problem hiding this comment.
Hi dmurat,
Thanks for updating the PR. Sorry if my previous review comments were unclear, but there are still a few changes I'd like you to make.
Other than these few changes, your PR looks good to merge, so if it's still not clear what I'd like you to change, don't hesitate to ping me.
lib/source-map-generator.js
Outdated
| generator.addMapping(newMapping); | ||
| }); | ||
| aSourceMapConsumer.sources.forEach(function (sourceFile) { | ||
| if (aSourceMapConsumer._mappings === '') { |
There was a problem hiding this comment.
Please remove this if-check. We should always execute the code inside this if-block, even if the mappings are not empty.
| new SourceMapConsumer(util.testMapEmptyMappingsRelativeSources)); | ||
| util.assertEqualMaps(assert, map.toJSON(), util.testMapEmptyMappingsRelativeSources_generatedExpected); | ||
| }; | ||
|
|
There was a problem hiding this comment.
We also need a test for the following scenario:
- We have two sources: foo.js and bar.js
- The mappings are NOT empty, but all the mappings refer only to foo.js.
- The test should assert that bar.js is also included in the list of sources.
Note that this test will fail with your current code, because it will only add bar.js if the mappings are empty.
…mapping refers to the single source only.
|
Ok, I believe that I've understood the issue. I've just updated pull request. Tnx for help :-) |
|
I'm sorry about the delay on this. I think this is fine. |
3 participants
This is a proposed resolution for issue 250.