Use forward slashes in source maps #175
Conversation
|
//cc @mzgoddard. I'd like your thoughts on this issue, too. Do you agree that it makes sense to use forward slashes exclusively in the |
|
@jmeas no problem, will do! I was just checking the tests at the moment. Before your changes I had 10 tests failing:
I'll try your changes and check again. |
Yikes! We should work on getting that to zero :) |
|
@jmeas I've just ran the tests after applying your patch, everything goes well now (except one failing test due the banner's newlines but that's completely unrelated here, I'll file it separately). |
|
💃 Great. I'll wait to do a release until the other issue is figured out, too, and Z gives his feedback :) |
|
It should be simple enough, it is not really an issue with the functionality, just the test. It is really weird that the test sends |
|
@jmeas Yeah. For source maps we should normalize to This PR looks good to me. 👍 |
|
|
||
| file = pathPrefix + basename; | ||
| // Convert paths to use forward slashes for sourcemap use in the browser | ||
| file = file.replace(/\\/g, '/'); | ||
|
|
||
| sourcesContent[file] = code; |
There was a problem hiding this comment.
The replace should be made here
|
Thanks for the input, Z! And that comment is a good reference; makes a lot of sense to me. I'll make the change above then merge. |
Use forward slashes in source maps
|
|
||
| file = pathPrefix + basename; | ||
|
|
||
| sourcesContent[file] = code; | ||
| // Convert paths to use forward slashes for sourcemap use in the browser | ||
| sourcesContent[file.replace(/\\/g, '/')] = code; | ||
| topLevel = UglifyJS.parse(code, { | ||
| filename: file, |
There was a problem hiding this comment.
This line should be normalized too, otherwise the sources will contain backslashes.
On windows, node's path api returns backslashes for path separators. Because the source-map library expects forward slashes in all cases, their relative path logic (specifically their "normalize" function) gives incorrect results when passing in backslash. The mozilla/source-map library won't change this, because they are actually expecting URLs as input - see mozilla/source-map#91 (comment). This fix is similar to the one made for grunt-contrib-uglify: gruntjs/grunt-contrib-uglify#175. This should fix issue gruntjs#110 and gruntjs#95.
On windows, node's path api returns backslashes for path separators. Because the source-map library expects forward slashes in all cases, their relative path logic (specifically their "normalize" function) gives incorrect results when passing in backslash. The mozilla/source-map library won't change this, because they are actually expecting URLs as input - see mozilla/source-map#91 (comment). This fix is similar to the one made for grunt-contrib-uglify: gruntjs/grunt-contrib-uglify#175. Standardized line endings for test fixtures by enforcing LF end of files for fixtures directory (through repository .gitattributes). This change was needed because the expected output of source maps are based on input files with LF, rather than CRLF. Before this change the tests were breaking due to git autocrlf on Windows. The difference between the source maps generated from input files using CRLF and the source maps generated from input files using only LF can't be normalized in the test, due to the nature of the changes: the offsets in the source map are changed, as well as the embedded source. These kinds of changes aren't as simple to normalize as just line feeds in output files - and such normalization would be too invasive, to the point of making the test less effective. This should fix issue gruntjs#110 and gruntjs#95 and all unit tests should now pass.
Fixes #173
@UltCombo, I don't have a Windows machine to test this on so I just kinda 'guessed' with this code. Would you mind doing testing when you've got time to see if this works as intended?