SourceMap: Replaces cwd usage in resolving paths

[am11]

Fixes the Ubuntu issue described here: sass/node-sass#399.

/cc @mgreter, @akhleung

[mgreter]

Maybe I'm not able to wrap my head around it, but this doesn't (IMHO) look right:

resolve_relative_path(real_path, source_map_file, File::dir_name(source_map_file))

string make_absolute_path(const string& path, const string& cwd)
{
  return (is_absolute_path(path) ? path : join_paths(cwd, path));
}

string resolve_relative_path(const string& uri, const string& base, const string& cwd)
{
  string absolute_uri = make_absolute_path(uri, cwd);
  string absolute_base = make_absolute_path(base, cwd);
...

This basically translates to (for make_absolute_path(base, cwd)):

string absolute_base = make_absolute_path(srcmap_file, dir_name(srcmap_file));
# if srcmap_file is not an absolute url => join_paths(dir_name(srcmap_file), srcmap_file);
# => srcmap_file = "path/to/file" -> absolute_base => path/to/path/to/file (what I would expect)

I tested your changes against my unit tests (only contains a very limited set of tests for source maps) and it didn't break. I'm probably missing something here! It's also not very clear what the "Ubuntu issue" actually is. Do you have some unit tests I could run to see the behaviour. If not, maybe you could add some to CSS-Sass!?

Had not much time at hand to go through it, but still wanted to give some feedback!

[am11]

In case of node-sass, the current working directory (cwd) is the directory where the node.exe is executed. Therefore, if the cwd is at some random directory (C:/program files/ etc.), it resolves the following relative paths incorrectly:

• from CSS to Map file. (used for sourceMappingURL comment in CSS)
• from Map to CSS file. (used for file member in .map)
• from Map to Source file. (used for sources array in .map)

With this commit:

• from CSS to Map file: cwd is set to output_path's directory
• from Map to CSS file: cwd is set to source_map_file's directory
• from Map to Source file: cwd is set to source_map_file's directory

In Ubuntu, these two test cases were failing. Supposedly, we have the following:

cwd: /vagrant/temp/path/
entry_point: ~/project/sources/file.scss
output_path: ~/project/file-output.css (in previous directory than source)
map_path: ~/project/source-maps/file-source-map.css.map

the current implementation (without this commit) will resolve the relative paths incorrectly.

[am11]

@akhleung, would it be possible to merge in #471, #476 and this one soon? Please review these, since node-sass will probably be picking up a release soon. :)

Thanks.

@andrew, could you please provide an ETA on vNext of node-sass? This PR will hopefully fix our CI build issue for 0.11.9. Also, is there any other issue we need to fix before vNext?

[mgreter]

I had some time today to run your failing test cases with node-sass.
I've added some code to context.cpp to see/debug the internal variables:

cerr << " === \n";
cerr << " ==   cwd: " << cwd << "\n";
cerr << " ==   out: " << output_path << "\n";
cerr << " ==  smap: " << source_map_file << "\n";

 ==   cwd: /tmp/node-sass/
 ==   out: /tmp/node-sass/test/out.css
 ==  smap: foo.css.map
  1) render to file should save the sourceMap to a specified file name:
     Uncaught AssertionError: false == true
# got: "../foo.css.map"
# expected: "foo.css.map"

Your patch changes the behaviour to resolve source maps relative to the output_path directory. IMO this could be a valid option, but should be discussed further! All other files are resolved to cwd currently. IMO your patch could lead to other problems since the third argument for resolve_relative_path should be already an absolute path, since it is passed as the second argument to make_absolute_path. This is not the case with File::dir_name.

// if real_path is not yet absolute, it will be resolved from source_map directory 
resolve_relative_path(real_path, source_map_file, File::dir_name(source_map_file))
// same logic applies for the `format_source_mapping_url`
resolve_relative_path(file, output_path, File::dir_name(output_path))

Hope this makes it a bit more clearer. I personally think the current behaviour feels more natural, as all other files are referenced from cwd. Implementors should still be able to resolve relative source-map paths to the output directory if they want. IMHO the new behaviour would not feel very intuitive (i.e. on the command line). But that's just my 2 cents, so the decision is for the project owners to make (@akhleung). There might be a cleaner coding approach, If this behaviour is prefered.

output_path: ./target/out.css
src_map_path: ./maps/out.map
  currently => ./maps/out.map
  with path => ./target/maps/out.map

Best regards

[akhleung]

Sorry, I've been really busy for the past several weeks. I'll take a look at these this week.

[am11]

@mgreter,

Thank you for taking time for debugging node-sass tests! 👍

(i.e. on the command line).

I totally agree with you. For this very reason, the implementer (of node-sass for instance) would let the CLI user to pass relative path for source-map, input and output and on calling the libsass' interface, they would have to resolve abs file paths. This in on my TODO to make sure there is no change in CLI usage in node-sass. The other upstream folks would also have to make sure all three paths are absolute paths and libsass would generate the right thing for them.

On the contrary, the cwd way is cleaner. But then the upstream implementer would have to take care of updating the paths in sourcemap file as well as reparse the CSS file to update the source-map comment (with regex!), which in my opinion is an expensive option.

From that perspective, it boils down to:

"resolving abs paths for three files input, output and source-map before calling libsass"

versus

"intervening with output generated by downstream (comes with costly payload)"

Thoughts?

[mgreter]

IMO you would only need to check if src_map_path is not absolute, and if it is not, make it absolute in relation to dirname of output_path. Therefore libsass should never try to make it relative to cwd on its own. All other files will resolve to cwd, and the cwd could be changed by implementor before invoking libsass. Or do I miss something? The paths for sourceMappingUrl and sources in source maps are still relative to each other.

[am11]

This is true! 👍

This issue is fixed in node-sass repo by am11/node-sass@45219ee via sass/node-sass#411.

This PR isn't required for node-sass now.

[mgreter]

👍