Synchronous custom importer not providing done

[ghost]

As I understand from the docs, a custom importer should be able to handle both render and renderSync by calling done() asynchronously. So a single importer can load files with the async readFile() and be handled correctly by both render and renderSync.

If this is correct then I'm seeing a bug as when I run the following code, done is showing up as undefined. This is what I believe is causing errors in the current implementation of eyeglass when run with renderSync which relies on calling done.

sass.renderSync({
    file: 'sass/test.scss',
    importer: function (uri, prev, done) {
        console.log(done); \\ logs undefined
    }
}, function(err, result) {
    console.log('successfully rendered');
});

[ghost]

Related to #849, linkedin/eyeglass#30

[ghost]

Some failing tests

[saper]

Looks like we pass the callback bridge as the third parameter (from C++, not to your JS callback), but the values are not used. Need to investigate.

[ghost]

So far I've tried https://github.com/DominicBarnard/node-sass/commit/812c5b88860f34fd4bc8ce128dc68c7c24c131b1 but my new tests are still failing :/
Error: file to import not found or unreadable: foo Current dir:

[saper]

You can cheat it this way saper@1417658

[ghost]

ok so that passes my tests now. But I expect that will break when an importer actually uses readFile to async load a file or does anything async?

[saper]

On Wed, 10 Jun 2015, Dominic Barnard wrote: ok so that passes my tests now. But I expect that will break when an importer actually uses `readFile` to async load a file?

Your importer might return before your callback gets a chance to return something meaningful... Needs to be tested, it's too late for me to guess

[ghost]

Yes this test is failing.

[ghost]

Do you think it's useful to support this or maybe importers that want to support renderSync should just stick to sync operations and returning results?

[saper]

On Wed, 10 Jun 2015, Dominic Barnard wrote: Do you think it's useful to support this or maybe importers that want to support renderSync should just stick to sync operations and returning results?

Definitely, you don't want to hang around waiting for all the callbacks to return only in order to bring back the results. What I am wondering right now is if there is a way to make an importer interface more helpful. Would "return" work for both async and sync cases? ~

[ghost]

As far as i understand it, if you only have return available, it can work for both render and renderSync but only synchronous operations will be possible inside the importer. So no possibility of say importing files from the network.

[ghost]

I think it's clean and well understood to have return for sync and done for async as long as developers know to only be async if they need to because they will loose support for renderSync and let the edge case of projects using renderSync and expecting to do async stuff like fetching from the network figure it out. Maybe just emit a meaningful error message from renderSync when nothing is returned that helps devs track a down a module which is trying to be async.

[saper]

I am sorry but we will drop this one. Do not mix sync and async interfaces.