-
Notifications
You must be signed in to change notification settings - Fork 362
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add check that URLs don't contain Windows path separators (\
)
#91
Comments
We can't rely on the path module because it only exists in node, not the web or inside firefox. The reason we use I'm not against supporting alternative path separators as long as it doesn't negatively affect the existing 99% use case. |
I mean, that if library user will set |
We can try to load |
This library takes URLs as input. A path is not a valid input if it's separated with Instead we should validate if the passed string is a valid URL. |
@sokra OK, no problem, I make filter in my app. But we really need validation, because with issue is in many apps. For example, Rework also send file name, not URL: https://github.com/reworkcss/css-stringify/blob/master/lib/source-map-support.js#L22 |
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.
\
)
\
)\
)
Updated title to reflect the expected behavior. |
On widows file paths will include
\
instead of/
andsource-map
doesn’t convert slashes, but source map will work only with UNIX slashes.We can fix it in app, but I think, that we need to fix it here, because every
source-map
node.js user must to repeat this fix.Autoprefixer issue: nDmitry/grunt-autoprefixer#25
I can helps with PR, but how we should do this? Try to load
path
(it wil be in node.js and browserfy) and ifpath.sep == "\\"
replace all\
to/
?The text was updated successfully, but these errors were encountered: