Improve custom importer to pass parent import context

[mgreter]

Needs some more work and more tests to make sure
it does not introduce any memory leaks!

One example would be a custom importer that loads the
content from the web. If this includes another import,
it should be loaded relative to the parent file. To do
this, the custom importer needs to know the context of
the parent import. Which is what this commit will fix!

[coveralls]

Coverage increased (+0.12%) when pulling 34a5752 on mgreter:feature/importer-parent-context into 0877bf0 on sass:master.

[mgreter]

I'm actually OK with merging this. The feature should work and I would like to get the API changes for the importer in ASAP. I initially thought about unifying all the needed info directly into sources (which seems to be the main storage variable for loaded files), so import_stack could just reference it.

Basically that would get rid of this:

sources.push_back(contents);
included_files.push_back(abs_path);
queue.push_back(Sass_Queued(load_path, abs_path, contents));
source_map.source_index.push_back(sources.size() - 1);
include_links.push_back(resolve_relative_path(abs_path, source_map_file, cwd));

But this can be postponed to a later point!
Content is only referenced by pointers (freed via sources)!

[xzyfer]

The C API is out of my wheel house so I completely trust your opinions here. Feel free to merge C API related changes/updates at will!

[am11]

@mgreter, I think the Perl-Example link on wiki is incorrect. I just updated libsass master, and its throwing an error here. Currently, I am passing pointer to struct Sass_Import** sass_importer(const char* file, void* cookie).

[mgreter]

Yep, updating the docs is on my todo list. There was a small change in that API: 34a5752

typedef struct Sass_Import** (*Sass_C_Import_Fn) (const char* url, const char* prev, void* cookie);

You probably only need to add const char* prev to your importer. This was added since you should do imports relative to the parent context. This will provide you the needed information to the path or url or whatever you gave it with the previous import.

[am11]

Great! So basically can I interpret it llike this:

// JS

// set options:
options.file = blah;
...
options.prefix = "some-url-prefix-string";

options.importer = function(file, prefix, done) {
  // do something with file and prefix and when you
  // are done producing result, call done(result)
};

Then in binding code, pass prefix as second parameter to Sass_C_Import_Fn.

I am afraid my example might not be accurate, but does it make sense from node-sass implementation viewpoint?

[mgreter]

I wouldn't call the variable prefix, since it contains also the filename of the previous import.
I guess it is not very obvious what this "feature" is about, so I try to give a small example:

Lets say we have this local scss file "import.scss":

@import "http://assets.example.com/styles.scss";
# custom importer gets called with:
# "http://assets.example.com/styles.scss", "import.scss", *cookie

The file on the server has this content:

@import "partial.scss";
# custom importer gets called with:
# "partial.scss", "http://assets.example.com/styles.scss", *cookie

Somce the second import is relative, it should be relative to its parent (which is on a remote server in this case). We added this string to make this information accessible to implementors.

With second thoughts it might have been better and easier to just have a function to query the import stack. IMO this feature is pretty important for custom importers, so I would like to ship it with the first release that includes the new API. Your opinions @am11, @xzyfer ?

[am11]

Oh that's actually the previous URL! But what would happen if partial.scss (in your example) also imports second-partial.scss, then we lose the desired context in prev?

I think it would probably make sense to make it projected_path, which returns the absolute path (or url) that libsass interpreted (as opposed to the first parameter url, which is the parsed path as is).

Thoughts?

IMO it makes sense to wait for this feature before the release.

[mgreter]

Good point, I guess the sample is still incomplete :)

@import "partial.scss";
# custom importer gets called with:
# "partial.scss", "http://assets.example.com/styles.scss", *cookie
# @return "http://assets.example.com/partial.scss", *loaded_data

Next import looks like this:

@import "second-partial.scss";
# custom importer gets called with:
# "second-partial.scss", "http://assets.example.com/partial.scss", *cookie
# @return "http://assets.example.com/second-partial.scss", *loaded_data

It may should be noted that you must include loaded_data in this case, since libsass will not know what to do with the url. I guess it currently will just try to load it from the filesystem.

It's not as sophisticated as the original ruby sass importer; but it's a start.

[am11]

typedef struct Sass_Import** (*Sass_C_Import_Fn) (
  const char* parsed_url,       // libsass: what I got
  const char* interpreted_url,  // libsass: my interpretation of what I got
  void* cookie                  // libsass: some cookie you want to eat later, I don't care about
);

😃

[am11]

I think beyond current URL and its interpretation by libsass, the tertiary implementer/consumer can maintain the list of imports (perhaps in a static memory) it has received by libsass via importer function -- if the use-case is extra-ordinary.

[mgreter]

I don't think I can follow where you want to go with that. Can you come up with a use case or example!?

I'm thinking about reverting the additional callback parameter and use functions to query the stack:

size_t sass_importer_get_stack_size();
const char* sass_importer_get_stack_url(size_t idx = 0);

I actually like this a lot better! Sorry for the detour @am11, but I really think I should have done it that way in the first place. Oh well, rome wasn't built in a day either ;)

[am11]

Haha! That is true.

Actually I am assuming that libsass without custom importer, does the following:

• If the initial input path is URL or absolute FS path, set cwd = directory_of(input_path).
• Subsequent paths in import chain gets resolved with cwd, unless another (a different) URL or abs path is encountered (in which case set cwd = directory_of(current_file)). I think the recursion plays a role to step in and out to get the correct-previous cwd, in case an SCSS document has multiple imports?

Now suppose abc.scss imports xyz.scss and 123.scss, then xyz.scss imports http://web/assets/foo.scss. Now final user doesn't know what order does the parser use? Depth-first or breath-first?

Momentarily, if it was a depth-first, then the previous to 123.scss was http://web/assets/foo.scss, which makes it less meaningful to the user because it wanted to deduce the correct absolute path.

So in my opinion, libsass should keep that context (resolving path with correct cwd based on the recursion step or tree or whatever) and send us the paths before and after the interpretation, along with cookie. I think this will suffice most of the use cases (the final implementer can then cache the URLs, store them in cache and make use of them depending on their use-case).

Does this make sense?

[am11]

Separately, I think (not sure if it is beyond the scope at this point), for downloading content in case of URL, libsass can probably make use of http://casablanca.codeplex.com/.

[am11]

(or even better libcurl: https://github.com/bagder/curl)

[mgreter]

I don't think libsass will support this anytime soon (probably never). And this is quite reasonable, since we want to concentrate on the sass part and not how to "interface" with the outside world. We like to have our tiny and well defined API and let the outside world mess up things :)

Unfortunately again I have to abandon my previous idea with the additional API functions.
Problem is, that this information is only stored in C++ currently. And it would take quite some time to make that available to the C-API. It might happen once the mess with all the status variable in sass context has been cleaned up.

So for now I will leave it as it is. IMHO you can do the correct stuff now with "nested" import calls in any way your context suits you. But I agree that this makes it impossible to access any "grand-parent" imports. Not sure if this really is a common use case!? For now we let the implementors do all the hard work, we only provide the raw information. I guess it's better than nothing!

[am11]

Thanks for clarification. Lets go with the current implementation. :)

I updated node-sass code with prev option. I think I have found two bugs 🐛 🪲

Consider the following setup:

C:\temp\alpha\index.scss:

@import 'foo.scss';
@import 'bar.scss';

C:\temp\alpha\foo.scss:

@import 'bar2.scss';
body {
color: red;
}

C:\temp\alpha\bar.scss:

p {
color: grey;
}

C:\temp\alpha\bar2.scss:

span {
z-index: 4;
}

Following is the JS code (lets call it script1):

require("./").render ({
  file: "c:/temp/alpha/index.scss",
  outFile: "c:/temp/alpha/index.css",
  sourceMap: "c:/temp/alpha/index.css.map",
  success: function(css, map){
     console.warn(css); console.warn(map);
  },
  error: function(err, status){
    console.error(status + ": " + err);
  },
  importer: function(file, prev, done) {
    console.log("File: " + file);
    console.log("Previous: " + prev);
    done({
      contents: "div {color: yellow;}"
    });
  }
});

(in the importer, I am printing the first two parameters as is to stdout)

Output for log messages:

File: foo.scss
Previous: c:/temp/alpha/index.scss
File: bar.scss
Previous: c:/temp/alpha/index.scss

Bug 1: bar2 never reached importer. The expected output was:

File: foo.scss
Previous: c:/temp/alpha/index.scss
File: bar2.scss
Previous: c:/temp/alpha/foo.scss
File: bar.scss
Previous: c:/temp/alpha/index.scss

Next, the generated source-map looks like this:

{
        "version": 3,
        "file": "index.css",
        "sources": [
                "index.scss",
                "../../../C:/temp/alpha/foo.scss",
                "../../../C:/temp/alpha/bar.scss"
        ],
        "sourcesContent": [],
        "mappings": "AEAA,AAAK,AAAO,AAAZ,AAAK,AAAO",
        "names": []
}

Bug2: The sources are messed up, although in the example above, I am only changing the content in importer (not the file path).

If I remove importer from script1, then it generates correct path (and also we have bar2 in sources array, which was missing previously):

{
        "version": 3,
        "file": "index.css",
        "sources": [
                "index.scss",
                "foo.scss",
                "bar.scss",
                "bar2.scss"
        ],
        "sourcesContent": [],
        "mappings": "AGAA,AACA,AAAS,AFAT,AACA,AAAO,ACFP,AACA,AAAO",
        "names": []
}

Can we nail them before the release? :)

[mgreter]

Thanks for checking it out. Will add your example to my other perl integration tests. If you are right this needs some more work. Will have time in some hours ...

[mgreter]

I tested your sample with perl-libsass and unfortunately it works correctly here:

• https://github.com/mgreter/perl-libsass/blob/api/c-interface/t/07_custom_importer.t
• https://travis-ci.org/mgreter/perl-libsass/builds/44367196

Are you sure you return the right thing from the importer? Looks like you don't!
As for the path, I would guess "c:/.../.../" is not a valid windows path (forward slashes).

[mgreter]

OK, I just pushed a commit to master which probably fixes at least some parts of the problem you have. I noticed you do not return a filename and this caused some problems on linux (which I wasn't aware off).

[am11]

Hi. I updated to latest libsass, but the sourcemap issue still exists.

It looks like in case of custom importers, the paths in source-map's sources[] array are not resolved w.r.t generated .map file path. Clipping out the importer function produces correct result (without changing anything else).

require("./").render ({
  file: "c:\\temp\\alpha\\index.scss",
  outFile: "c:\\temp\\alpha\\index.css",
  sourceMap: "c:\\temp\\alpha\\index.css.map",
  success: function(css, map){
     console.warn(css); console.warn(map);
  },
  error: function(err, status){
    console.error(status + ": " + err);
  }
});

vs

require("./").render ({
  file: "c:\\temp\\alpha\\index.scss",
  outFile: "c:\\temp\\alpha\\index.css",
  sourceMap: "c:\\temp\\alpha\\index.css.map",
  success: function(css, map){
     console.warn(css); console.warn(map);
  },
  error: function(err, status){
    console.error(status + ": " + err);
  },
  importer: function(file, prev, done) {
    console.log("Previous: " + prev);
    done({
      contents: "div {color: yellow;}"
    });
  }
});

BTW, node.js runtime does not mind Linux-style slash in Windows. :)
Also, cd c:/temp/alpha/ and even cd /temp/alpha are valid and equivalent in powershell (when $pwd is in C: drive). In cmd, it only recognizes back-slash, so cd c:\temp\alpha\ and cd \temp\alpha are both valid and pointing to same directory (if cd , is in C: drive).