Provide api for custom importers

[jhnns]

With sass/libsass#21 libsass will provide the possibility to specify a custom importer. This is useful for build tools like webpack that provide a custom resolving algorithm.

We basically just have to wrap the js-callback so it can be called by libsass:

sass.render({
    ...
    importer: function (filename) {
        // rewrite filename
        return filename;
    }
});

The libsass-api is a bit more complex to support more advanced use-cases as described here. I'm planing to make the js-api a bit more pleasant to work with.

For example you could do:

    importer: function (filename) {
        return filename;
    }

    importer: function (filename) {
        return {
            filename: "..."
            filesource: "..."
        };
    }

    importer: function (filename) {
        return [
            {
                filename: "..."
                filesource: "..."
            },
            {
                filename: "..."
                filesource: "..."
            }
        ];
    }

One problem I currently see is that if a filesource is provided, this needs to be done synchronously which might be impossible in certain situations.

What do you think about it? If it's ok for you I could start a pull-request.

[keithamus]

@jhnns looks like a cool feature, thanks for raising the issue here.

Before you go ahead and make a PR though, I'd wait to see what happens with libsass. The issue hasn't been closed yet - and it looks like no implementation has been done on the libsass side of things. Let's wait for that and then focus on getting it implemented in node-sass 😺

[jhnns]

Yep, sure. There's especially a problem with asynchronousness.

[callumlocke]

@jhnns why does it need to be done synchronously? Is there no way we can do this with a callback?

    importer: function (filename, callback) {
        callback({
            filename: "..."
            filesource: "..."
        });
    }

[jhnns]

The problem is, that the C-code needs to be paused until node called back with a result. I don't know if that is possible with libuv.

It would be possible if libsass provided a callback.

[jhnns]

The api is implemented in libsass now. I guess I'll implement the easiest use-case for now: An api to rewrite the import paths.

[am11]

@jhnns, with V8 API, this can be done in a async callback manner. I have couple of ideas how to implement in our binding. For one, see http://stackoverflow.com/a/13904693/863980.

Going by your example:

importer: function (filename) {
  return {
    filename: "..."
    filesource: "..."
  };
}

I am not sure if it allows to set source-content (but only the souce-file paths). At least I can't find that in the new API docs. However, we can make it so to get the sourceContents back in importer's callback. But the sole purpose of importer is to feed the import files to libsass, isn't it?

Here call back seems like we have another candidate to success event, which would be redundant / incorrect.

Perhaps, @mgreter can shed some light on this: what is the use case for the callback in import function?

Nonetheless, please wait till #543 is merged, or you can start by pulling my master branch, since the current binding code is rendered obsolete.

[mgreter]

Not really sure what the question really is. You get called for each import statement and can return either the filename with or without its source (libsass will try to load it itself if no source is provided). It's as simple as that. Not sure what the confusion about this hook/callback is!?

[am11]

Thanks! Actually, I am confused about the use case of void* cookie function pointer, which would eventually be used as callback.

[mgreter]

OK, let me try to explain it. The cookie can be used to store anything you'd like with the importer (it's completely up to the implementer). Maybe you want to attach some configuration that can be used inside the importer. Or you can store a function reference to a node.js function and call it inside the importer and use the return value. You probably will not be able to use node.js function directly for the importer callback, since you pretty sure will need to convert some data between C and node.js before/afterwards.

[am11]

Oh ok, so basically, its scope is just confined to when our importer is called. Perfect! 👍

I will try to implement it, so we let user pass a JS object, which can contain key-value pair, where value can be an inline / local function.

[am11]

@mgreter, I have pushed the code in a separate branch: am11@00057b7.

The compilation is succeeding but apparently it is not calling the JavaScript function.

We are expecting either JS object literal { } or array of objects [ { }, { } ..], any other (top-level) data type returned should be disregarded:

//# this is nodejs interactive console, pwd == node-sass' root directory

require("./").render({
  file: "C:/temp/example.scss",
  success: function (css,map) { console.log("css: " + css); console.log("map: " + map) },
  error: function (err) { console.log("err: " + JSON.stringify(err)) },
  importer: function (file) { console.log("importer: " + file); return file; }
})

Changing return file to return { path: file } should take path into account.

Here is another example, a valid case with both path and source set:

//# this is nodejs interactive console, pwd == node-sass' root directory

require("./").render({
  file: "C:/temp/example.scss",
  success: function (css,map) { console.log("css: " + css); console.log("map: " + map) },
  error: function (err) { console.log("err: " + JSON.stringify(err)) },
  importer: function (file) {
    console.log("importer: " + file);
    return { 
      path: '/my/random/file.scss',
      contents: 'body{ color: red }'
    };
  }
})

The debug output is never printed to the console. I need to dig it further what's going wrong.

[mgreter]

I'm currently looking into it, but first it just segfaultet for me because, as it seems, ctx_w is not always defined? importer_callback seems to be set only if ctx_w is available, therefore I guess it contains random garbage and segfaults because it detects the pointer to lay outside the allowed memory pages.

if (ctx_w) sass_option_set_importer(sass_options, sass_make_importer(sass_importer, ctx_w->importer_callback));

Will now check if I can get the basic example working ...

[am11]

Oh yes! For now, you can say, it is just implemented for async methods only (not the renderSync and renderFileSync). :)

[mgreter]

So you mean for sync methods only? For me a basic example works, sass_importer is getting called and the cookie is passed correctly. From there I cannot really help much as why your node.js function is not called.

[am11]

Fixed ctx_w issue with am11@d7c80a0 am11@3a9c84b.

Async methods (without sync suffix). Once this gets working, later we would implement for sync methods (which would require extra steps to compile and validate JavaScript code before hooking it; for example, users might be able to use --importer "function(file){ /* random stuff and then */ return file; }" from CLI).

[jhnns]

@am11 Ok, then I'll do nothing until your rewrite is done. Or are you implementing the importer API anyway (I'm ok with that)?

I'm not experienced enough with v8 and libuv, just wanted to ensure if an async importer will be possible?

    importer: function (filename, callback) {
        doSomeAsyncTask(function (err) {
             callback({
                 filename: "..."
                 filesource: "..."
             });
        });
    }

[am11]

@jhnns, I used the void* cookie to pass the C++ binding's callback function. From that callback it is invoking JS function and passing the input_path.

IMO, it's better that we keep it like this:

importer: function (inputPath) {
 return callSomeSyncFunc(inputPath);  // returns object: {path, content}
}

As working on objects in synchronous manner is comparatively straight-forward to implement.

[jhnns]

Well, the problem is that a synchronous importer is not always possible. For example you can't do a synchronous http request. Don't know if anybody wanted to that but if you're bound to do something synchronously there are not many possibilities in node.js.

For now a synchronous api is ok. But in that case you probably only want to rewrite the path. I can't imagine a use-case where you want to pass the filesource without doing something asynchronously.

[am11]

@mgreter, while debugging importer branch with the first chunk of JS code I posted above, it throws Memory Access violation exception in Context::parse_file for queue.

See:

And here is the call stack:

>   binding.node!Sass::Context::parse_file() Line 248   C++
    binding.node!sass_parse_block(Sass_Context * c_ctx, Sass::Context * cpp_ctx) Line 372   C++
    binding.node!sass_compile_context(Sass_Context * c_ctx, Sass::Context::Data cpp_opt) Line 408   C++
    binding.node!sass_compile_file_context(Sass_File_Context * file_ctx) Line 478   C++
    binding.node!compile_it(uv_work_s * req) Line 14    C++

Note: (see the dropdown next to ":arrow_forward: Continue" says Release ?) I had to change the configuration mode to Release as it was throwing me HEAP_CORRUPTION DETECTED in Debug mode on freeing char** arr in free_string_array( _ ) function.

[am11]

@mgreter, the queue issue is fixed with the latest commits. Thanks!

Could C++11 lambdas be used as function pointers, it would have made our callback code more rhetorical.

Nonetheless, I have a breakthrough in calling and getting back JS results using libvu! am11@cbcacc2. (finally 😄)

The next issue is with the uv_async_send, which requires the actual JS-context-aware loop, which we have and we can provide. But then it waits for the main thread to run.. which defeats the purpose.

[am11]

Status: It worked! ⚡

Credit goes to @txdv and @rvagg. Thank you guys for all the tips!

Code is in my importer branch.

Next, we need to find a way to support the JS callback function, which @txdv explained why it is necessary (to enable users run async functions within the importer function). My first try failed, because Handle<Value> argv[] naturally requires Handle<Value> (or we can perhaps change it to Local<Value> at max), while we need to send the Local<Function> reference, which we first need to capture (somehow?).

@rvagg, in order to store and forward the JS callback function reference, (without any modification in C code), is there a concise/neat way to do so using NanNew<Function> I suppose? We can't define FunctionTemplate, because we don't have its params context there and we don't need it I guess.

For instance:

/** 
 * @param {String} file
 * @param {Function} jsImporterCallback
 *
 * @return {Object} modifiedImport
 */ 
importer: function (file, jsImporterCallback) {
  // this function is called by libuv intermittently, but synchronously, as in; one at a time
  // do some magic with "file" string, make use of jsImporterCallback and return back to libuv

  var modifiedImport = {
    file: alteredVersionOf_file,
    contents: contentReadFromSomeResource; // could be read from DB, web-service, file
                                           // or any string containing SASS code
  };

  return modifiedImport;
}

In the above code, I am referring to the parameter jsImporterCallback, which we need to send back to JS as is. This would always be the second parameter and we can't guarantee the parameter name would remain the same; perhaps user would like to specify function(path, cb).

Having said that; do we need to cast it to Handle<Value>? Given we already have NanCallback for importer: function available.

[rvagg]

See my comments inline about how sleep() isn't going to be an acceptable path forward here and will lead to edge-case bug reports (likely much more common than you suspect!).

Also see NanAsyncWorker in NAN for a convenient way to store callback and other JS objects for when you come back to eventually make a callback after invoking a job on a worker thread. The SaveToPersistent() and GetFromPersistent() API does this. When you construct test cases for this, try and take into account the case where the GC could potentially be getting hold of objects and cleaning them up during an async call, this is a common edge-case with async work needing state after the work is complete.

e.g.

function invoker () {
  var importantThing = 'very important'
  var arg = 'some argument'
  asyncWork(arg, function asyncCallback (err) {
    otherFn(err, importantThing)
  })
}

If you follow that through from a GC perspective, the only thing that has a reference to arg and asyncCallbak (and therefore importantThing) is whatever is happening in asyncWork() and if that's a C++ addon function and it doesn't save asyncCallback to a Persistent then if the async work takes long enough it's likely that the garbage collector will clean up both asyncCallback and importantThing with it and when you go to invoke the callback from C++ you're invoking an empty handle which won't even report an error, it just has no effect, which is annoying to debug! NanAsyncWorker saves the callback for you and you can also put in any other property to it, for instance if you needed arg to be stick around until after the callback for some reason then you could SaveToPersistent().

[am11]

Hello @rvagg, thanks for your detailed input. I now realize how important is it to avoid race condition in such a situation. The good news is, this is fixed by using uv_cond_t and uv_mutex_t: am11@2e82e70. Now there is no race condition, simple libuv signaling (like C++11's std::condition_variable) to get status of task dispatched to the main libuv (default_loop) thread.

@jhnns I am still (again) confused, what is the purpose of jsImporterCallback in importer: function (file, jsImporterCallback). I couldn't draw an example to work with. Can you provide an example, where it is imperative to have a callback as parameter in importer function, and "no callback support" won't cut it? The thing is, importer in JS is already a callback like success and error, so we are not calling it or passing anything to it in JS space.

The following test is now working:

//# in node interactive console
//# process.cwd == node-sass repo dir

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) {
    console.log("I am in importer, yay!");
    return {
      file: "/some/random/path/file.scss",  // non-existent, but it doesn't matter due to the next line
      contents: "div {color: yellow;}"
    }
  }
});

If that suffice the purpose, we can merge in the importer branch.

[jhnns]

Since node.js is single-threaded – at least in the JS world – most APIs are async by default. Take the fs-API for example:

var fs = require("fs");

sass.render({
  ...
  importer: function (file) {
    fs.readFile(file, "utf8", function (err, content) {
      // oops, we're too late, function has already returned
    });

    return {
      file: "/some/random/path/file.scss",
      contents: ? // we can't pass anything here yet
    };
  }
});

Luckily there is a sync-version for every fs-method:

  importer: function (file) {
    var content = fs.readFileSync(file, "utf8");

    return {
      file: "/some/random/path/file.scss",
      contents: content
    };
  }

But that's an exception, most I/O APIs are asynchronous. What if we wanted to perform an HTTP request?

  importer: function (url) {
    http.get(url, function(res) {
      // damn, we're too late again!
    });

    return {
      file: "/some/random/path/file.scss",
      contents: ?
    };
  }

There is no http.getSync...

[am11]

Ok, how will you pass callback to importer in this case?
If you want to do it in the aforementioned test case.

[keithamus]

@am11 I think the point is that if importer was passed a callback, we could do async stuff, e.g.:

  importer: function (file, callback) {
    fs.readFile(file, "utf8", function (content) {
        callback(null, {
            file: "/some/random/path/file.scss",
            contents: content
        });
    });
  }

  importer: function (url, callback) {
    http.get(url, function(res) {
      callback(null, {
          file: url,
          contents: res.body,
       });
    });
  }

[am11]

Aaaaaand..... its done()! 😄 🎉

Here we go: fd8c7f7

I have used std::vector<> in binding.cpp file, don't know if that is the brilliant way to do it.

I will throw in couple of test cases for it subsequently.

Now you can say:

//# in node interactive console
//# process.cwd == node-sass repo dir

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 + ": " + JSON.stringify(err)); // see #543, errors serialized as JSON.
  },
  importer: function(file, done) { // you can name params as you please
    console.log("I am in importer, yay!");
    done({
      // each tuple consists of file and content
      // meaning, here done() can take array
      // of objects [{file:'',content:''},{file:'',content:''}, ..]
      file: "/some/random/path/file.scss",
      contents: "div {color: yellow;}"
    });
  }
});

Thank you all for your help and patience!

[txdv]

congrats

[jhnns]

Aweseome!! 👍

I was afraid that async importers are not possible. Need to dig the source code, I'm curious how you've managed it. 😀

Is there any roadmap for v2?

[am11]

As soon as libsass gets new release. See sass/libsass#697.

I actually read @rvagg's NAN repo's README this time. :)

Then used synchronous example and drew the code from there. Once the synchronous method is called (done ()), it signals libuv to continue (via conditional variable).

Not the pure Nan-based solution, which @rvagg mentioned above, but fiddling with libuv mixed with Nan helped us achieved the target. Nonetheless, we can always improve things over time..

[mgreter]

I guess the code puts libsass in its own thread, so it can be stopped and continued at will, without interfering with the main event loop (which I guess is what libuv does)!?
Really had some doubts if you could pull that one off 😄 👍

[am11]

With 356d256, it now works with renderSync as well. Based this test case: https://gist.github.com/am11/1b38995e42e9d33bdd9b.
@txdv thanks again for your help.

On a slightly related note, based on #547, we should probably return both css and map in case of renderSync as well. So var value = renderSync(blah); would make value looks like {css: 'blah', map: 'blah'}. Currently, it just returns css string.

[jhnns]

If you change the api like this, you should probably also return the stats-object which I've introduced with #290. This pull-request introduced the possibility to pass an object which are filled with information about the compilation.

I've implemented it this way because the function did return only a string and I didn't want to change the api. But now – with your changes – I think it makes more sense to return the stats because they are the result of the compilation. You probably also don't need to nest these values in a stats-object at all and just return them on the same level as css and map.

renderSync(blah); 
// returns
{
    css: "..."
    map: "..."
    importedFiles: [] // I've renamed them to importedFiles
                      // in the style of the new importer option
}

[am11]

stats would works as is with sync:

var myStats = {};

var result = sass.renderSync ({
  file: "c:/temp/alpha/index.scss",
  outFile: "c:/temp/alpha/index.css",
  sourceMap: "c:/temp/alpha/index.css.map",
  stats: myStats,
  importer: function(file, done) {
    done({
      file: "/some/random/path/file.scss",
      contents: "div {color: yellow;}"
    });
  }
});

// result.css returns css string
// result.map returns source-map string
// if there is an error, it'll be thrown to stdout as JSON (object literal like, not stringified)

Analogous to its async variant:

var myStats = {};

sass.render ({
  file: "c:/temp/alpha/index.scss",
  outFile: "c:/temp/alpha/index.css",
  sourceMap: "c:/temp/alpha/index.css.map",
  stats: myStats,
  success: function(css, map) {
    // css is string
    // map is string
    // mystats is JS object literal
  },
  error: function(err, status){
    // status is an integer
    // err is JS object literal (parsed JSON, see #543)
  },
  importer: function(file, done) {
    done({
      file: "/some/random/path/file.scss",
      contents: "div {color: yellow;}"
    });
  }
});

I have made the changes in my simplified-api branch: 125f646.

Appreciate your input. Lets continue discussion on this at #547.

[txdv]

@am11 This time I didn't help much, however I want to add my two cents:

IMO in renderSync the importer function should use "return { }" instead of a done callback. This is a better paradigm for blocking functions. Now you are mixing them.

[am11]

@txdv, the binding code would remain the same as the uv call will be deferred to main loop and wait for JS to return.

It is similar to how mocha.js testing framework works; gives full control to user by passing the done callback on, and letting the user decide whether to use it asynchronously or not. We can certainly restrict it in our JS code (by keeping the done() propagating to end-user), but I don't see a significant gain there except for the fact that it is strictly logical. :)

[am11]

The final version looks like this:

stats = {};
require("./").renderSync ({
  file: "c:/temp/alpha/index.scss",
  outFile: "c:/temp/alpha/index.css",
  sourceMap: "c:/temp/alpha/index.css.map",
  stats: stats,
  importer: function(file, prev, done) {
    console.log("Previous: " + prev);
    done({
      file: "/some/random/path/file.scss",
      contents: "div {color: yellow;}"
    });
  }
});

Where prev is the previously resolved (absolute) path, sometimes used to resolve the current path. More details here: sass/libsass#691. Down in comments, you will find some known source-map issues. But for most part, it works quite well.

[jhnns]

I have to agree with @txdv: It looks strange, to use an async function signature within a synchronous call (and I didn't know that this is possible). But if it's working, I don't know why we should restrict the developer to synchronous functions.

Is it still possible to just return the value? Like mocha is providing the possibility to either return a value or accept a done() callback for async tasks.

[am11]

@jhnns, good idea! See: 471a075.
Now you can return value from importer or use callback regardless of render() or renderSync().

[am11]

The catch is, it does not reflect what underlying C++ implementation is doing: calling the Nan callback anyway.

[jhnns]

Cool 👍

[am11]

v2.0.0-beta is just released.

Thanks for all your support guys!

[jhnns]

Nice! Then we can update the sass-loader