-
Notifications
You must be signed in to change notification settings - Fork 349
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
Don't canonicalize file case in source maps #541
Conversation
Adding tests first, without implementation, to verify that they fail on Windows and thus that the fix actually does something. |
6ac1cb6
to
514b263
Compare
This will already have been canonicalized by the import process to whatever degree the importer deems necessary.
5e0bd7f
to
d9e99f8
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM with one comment.
@@ -27,7 +27,9 @@ class FilesystemImporter extends Importer { | |||
ImporterResult load(Uri url) { | |||
var path = p.fromUri(url); | |||
return ImporterResult(io.readFile(path), | |||
sourceMapUrl: url, syntax: Syntax.forPath(path)); | |||
sourceMapUrl: | |||
io.couldBeCaseInsensitive ? p.toUri(io.realCasePath(path)) : url, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could omit this check here since realCasePath()
makes a point to do the right thing. I guess this avoids converting to back to a uri from a string but it feels awkward.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, efficiency is the goal here. URI parsing is probably dwarfed by file IO, but this importer is on a fairly hot path and little things can add up. I'll add a comment explaining why we're doing this.
Closes #540