Skip to content

Conversation

@mzgoddard
Copy link
Contributor

While improving gruntjs/grunt-contrib-concat#59 after @lydell's insight to use the third parameter of SourceMapGenerator#applySourceMap I noticed the sourcesContent was not being honored in my test cases. sourcesContent would be present, but the array was all null values. To fix that, I have added a small branch similar to that in the this._mappings.forEach block in applySourceMap. To test it, I modified the related test for the second and third parameter of applySourceMap to test if it is supporting sourcesContent.

Set source content with relation to aSourceMapPath in
SourceMapGenerator#applySourceMap. Before, in the related test,
`../coffee/foo.coffee`'s source content would be placed at
`../coffee/foo.coffee` instead of where it should be at
`coffee/foo.coffee` derived maps. Not being stored at that location it
was left out of the generated map.
@lydell
Copy link
Contributor

lydell commented Mar 10, 2014

Nice catch! LGTM.

@vladikoff
Copy link

Plus one.

@jamesplease
Copy link
Contributor

ping

@MarcDiethelm
Copy link

@fitzgen Please, can you take a look at this? Merging this would make a lot of people happy downstream.

@simonoff
Copy link

any updates?

@fitzgen
Copy link
Contributor

fitzgen commented May 31, 2014

Hey sorry, I've been pretty focused on other work, will look at this soon.

@fitzgen
Copy link
Contributor

fitzgen commented Jun 2, 2014

Looks good, thanks!

Sorry again about the delay.

fitzgen added a commit that referenced this pull request Jun 2, 2014
Fix applySourceMap's aSourceMapPath parameter source content support
@fitzgen fitzgen merged commit 2acc6ed into mozilla:master Jun 2, 2014
@mzgoddard
Copy link
Contributor Author

😄

@MarcDiethelm
Copy link

Thanks @fitzgen for reacting favorably to my tweet. ;)
Has this been published?

@fitzgen
Copy link
Contributor

fitzgen commented Jun 4, 2014

Not published yet. I was hoping to get a few of the other PRs merged as well and do it in one fell swoop. Will add a todo item to my list and if the other PRs don't get merged by the time I get to it I'll just publish anyways.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants