Skip to content
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

sass.node.js overoptimized ... #122

Closed
thiscantbeserious opened this issue Apr 8, 2019 · 3 comments
Closed

sass.node.js overoptimized ... #122

thiscantbeserious opened this issue Apr 8, 2019 · 3 comments

Comments

@thiscantbeserious
Copy link
Contributor

thiscantbeserious commented Apr 8, 2019

As per #88 you overdid it a little by hard-replacing the resolver path return value with .replace(/\\/g, '/'); in function resolve(request) { ... }

path.normalize does already return the correct normalized path for the specific target environment

When multiple, sequential path segment separation characters are found (e.g. / on POSIX and either \ or / on Windows), they are replaced by a single instance of the platform-specific path segment separator (/ on POSIX and \ on Windows). Trailing separators are preserved.

The only reason a path like C:/browser/test.txt works with many modern browsers is because they actually parse it with the Posix syntax (and their file:// protocol implementation). Technically those paths are incorrect everywhere else - like in NPM and will return a file not found error (like it currently does e.g. when used in conjunction with Gulp on Windows, where all @import rules fail).

Most if not all browser correctly unwrap paths like C:\test\file.txt to their correct syntax the browser expects, in this case "file://C:/test/file.txt".

So the correct solution should be to simply return the value that path.normalize returns without any hard replace applied.

Sorry for the long text, I hope it makes sense.

@rodneyrehm
Copy link
Member

do you want to send a PR for that?

@thiscantbeserious
Copy link
Contributor Author

Done :)

@rodneyrehm
Copy link
Member

This is fixed in release 0.11.0. thanks for your support!

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

No branches or pull requests

2 participants