Use proper comment format for css with pre-existing comment. #364
Conversation
If a pre-existing sourceMappingURL comment is loaded the write process always used `.js` format without a preceding line-feed. This alters the logic so sources with pre-existing sourceMappingURL is written with the correct type of comment for the output file. Fixes #363
|
I think the drop in coverage is due to removal of lines. In theory we should add a test for css with existing coverage map, though I'm unsure how to create such a test file. |
|
Have just tested this and looks good to me! 🎉 Thank you |
| commentFormatter = function(url) { | ||
| return ''; | ||
| }; | ||
|
|
||
| if (file.sourceMap.preExistingComment){ | ||
| debug(function() { return 'preExistingComment commentFormatter'; }); | ||
| commentFormatter = function(url) { |
There was a problem hiding this comment.
This is a major change, I need to look into what the preExistingComment handle was for. But your no longer returning a function here. Seems like we're missing a test that needs to exist.
There was a problem hiding this comment.
For current code preExistingComment prevents returning a string that starts with a line-feed. The existing code was fine for JS but not for CSS (// isn't a comment in CSS). We have a test for JS with a pre-existing comment but not for CSS with pre-existing comment. Honestly I'm not sure how to create a CSS file with a valid pre-existing source-map comment.
I'm not sure that it's very important to avoid adding a blank line between the source and the new comment, but the newlinePre variable is used to avoid that blank line and maintain current behavior.
| @@ -24,29 +24,27 @@ var getCommentFormatter = function (file) { | |||
| var extension = file.relative.split('.').pop(), | |||
| fileContents = file.contents.toString(), | |||
| newline = detectNewline.graceful(fileContents || ''), | |||
| newlinePre = newline, | |||
There was a problem hiding this comment.
Why do we need another variable which is the same thing. Just keep it as newline.
| } | ||
|
|
||
| switch (extension) { | ||
| case 'css': | ||
| debug(function() { return 'css commentFormatter';}); | ||
| commentFormatter = function(url) { | ||
| return newline + "/*# sourceMappingURL=" + url + " */" + newline; | ||
| return newlinePre + "/*# sourceMappingURL=" + url + " */" + newline; |
| return "//# sourceMappingURL=" + url + newline; | ||
| }; | ||
| return commentFormatter; | ||
| newlinePre = ''; |
There was a problem hiding this comment.
Why not?
commentFormatter = function(url) {
return newLine + "//# sourceMappingURL=" + url + newline;
};
return commentFormatter;
}
|
I'm going to merge this locally and fix any outstanding issues. |
|
Closing in favor of #366 ; @benjclark AND @coreyfarrell please see if the PR is good for you . If so I will merge and publish the patch. |
If a pre-existing sourceMappingURL comment is loaded the write process
always used
.jsformat without a preceding line-feed. This alters thelogic so sources with pre-existing sourceMappingURL is written with the
correct type of comment for the output file.
Fixes #363