Importer: General overhauling.. :bug::beetle:

* Fixes the async crashing by explicitly
  closing async.
* Changes the behavior of renderSync importer
* Updates index accordingly.
* Removes obsolete tests accordingly.
BIG THANKS to @txdv and this addresses #586 ! :tada::+1:

bin/node-sass

@@ -109,9 +109,7 @@ function getEmitter() {
     console.log(data);
   });
 
-  emitter.on('done', function(){
-    process.exit(0);
-  });
+  emitter.on('done', process.exit);
 
   return emitter;
 }

lib/index.js

@@ -144,7 +144,6 @@ function getOptions(options) {
 
   var error = options.error;
   var success = options.success;
-  var importer = options.importer;
 
   options.error = function(err, code) {
     try {
@@ -173,23 +172,6 @@ function getOptions(options) {
     }
   };
 
-  if (importer) {
-    options.importer = function(file, prev, key) {
-      var done = function(data) {
-        binding.importedCallback({
-          index: key,
-          objectLiteral: data
-        });
-      };
-
-      var result = importer(file, prev, done);
-
-      if (result) {
-        done(result);
-      }
-    };
-  }
-
   delete options.image_path;
   delete options.include_paths;
   delete options.includePaths;
@@ -219,6 +201,25 @@ var binding = require(getBinding());
 module.exports.render = function(options) {
   options = getOptions(options);
 
+  var importer = options.importer;
+
+  if (importer) {
+    options.importer = function(file, prev, key) {
+      function done(data) {
+        binding.importedCallback({
+          index: key,
+          objectLiteral: data
+        });
+      }
+
+      var result = importer(file, prev, done);
+
+      if (result) {
+        done(result);
+      }
+    };
+  }
+
   options.data ? binding.render(options) : binding.renderFile(options);
 };
 
@@ -232,6 +233,14 @@ module.exports.render = function(options) {
 module.exports.renderSync = function(options) {
   options = getOptions(options);
 
+  var importer = options.importer;
+
+  if (importer) {
+    options.importer = function(file, prev) {
+      return { objectLiteral: importer(file, prev) };
+    };
+  }
+
   var status = options.data ? binding.renderSync(options) : binding.renderFileSync(options);
   var result = options.result;
 

src/binding.cpp

@@ -4,7 +4,7 @@
 
 char* CreateString(Local<Value> value) {
   if (value->IsNull() || !value->IsString()) {
-    return const_cast<char*>(""); // return empty string.
+    return 0;
   }
 
   String::Utf8Value string(value);
@@ -15,6 +15,41 @@ char* CreateString(Local<Value> value) {
 
 std::vector<sass_context_wrapper*> imports_collection;
 
+void prepare_import_results(Local<Value> returned_value, sass_context_wrapper* ctx_w) {
+  NanScope();
+
+  if (returned_value->IsArray()) {
+    Handle<Array> array = Handle<Array>::Cast(returned_value);
+
+    ctx_w->imports = sass_make_import_list(array->Length());
+
+    for (size_t i = 0; i < array->Length(); ++i) {
+      Local<Value> value = array->Get(i);
+
+      if (!value->IsObject())
+        continue;
+
+      Local<Object> object = Local<Object>::Cast(value);
+      char* path = CreateString(object->Get(String::New("file")));
+      char* contents = CreateString(object->Get(String::New("contents")));
+
+      ctx_w->imports[i] = sass_make_import_entry(path, (!contents || contents[0] == '\0') ? 0 : strdup(contents), 0);
+    }
+  }
+  else if (returned_value->IsObject()) {
+    ctx_w->imports = sass_make_import_list(1);
+    Local<Object> object = Local<Object>::Cast(returned_value);
+    char* path = CreateString(object->Get(String::New("file")));
+    char* contents = CreateString(object->Get(String::New("contents")));
+
+    ctx_w->imports[0] = sass_make_import_entry(path, (!contents || contents[0] == '\0') ? 0 : strdup(contents), 0);
+  }
+  else {
+    ctx_w->imports = sass_make_import_list(1);
+    ctx_w->imports[0] = sass_make_import_entry(ctx_w->file, 0, 0);
+  }
+}
+
 void dispatched_async_uv_callback(uv_async_t *req) {
   NanScope();
   sass_context_wrapper* ctx_w = static_cast<sass_context_wrapper*>(req->data);
@@ -42,39 +77,47 @@ struct Sass_Import** sass_importer(const char* file, const char* prev, void* coo
 
   ctx_w->file = file ? strdup(file) : 0;
   ctx_w->prev = prev ? strdup(prev) : 0;
-  ctx_w->async.data = (void*)ctx_w;
-  uv_async_send(&ctx_w->async);
 
   if (ctx_w->success_callback) {
     /*  that is async: Render() or RenderFile(),
      *  the default even loop is unblocked so it
      *  can run uv_async_send without a push.
      */
+    ctx_w->async.data = (void*)ctx_w;
+    uv_async_send(&ctx_w->async);
     uv_cond_wait(&ctx_w->importer_condition_variable, &ctx_w->importer_mutex);
   }
   else {
-    /*  that is sync: RenderSync() or RenderFileSync,
-     *  we need to explicitly uv_run as the event loop
-     *  is blocked; waiting down the chain.
-     */
-    uv_run(ctx_w->async.loop, UV_RUN_DEFAULT);
+    NanScope();
+
+    Handle<Value> argv[] = {
+      NanNew<String>(strdup(ctx_w->file ? ctx_w->file : 0)),
+      NanNew<String>(strdup(ctx_w->prev ? ctx_w->prev : 0)),
+      NanNew<Number>(imports_collection.size() - 1)
+    };
+
+    Local<Object> returned_value = Local<Object>::Cast(NanNew<Value>(ctx_w->importer_callback->Call(3, argv)));
+
+    prepare_import_results(returned_value->Get(NanNew("objectLiteral")), ctx_w);
   }
 
   return ctx_w->imports;
 }
 
 void ExtractOptions(Local<Object> options, void* cptr, sass_context_wrapper* ctx_w, bool isFile, bool isSync) {
+  NanScope();
+
   struct Sass_Context* ctx;
 
   NanAssignPersistent(ctx_w->result, options->Get(NanNew("result"))->ToObject());
 
   if (isFile) {
-    ctx = sass_file_context_get_context((struct Sass_File_Context*) cptr);
     ctx_w->fctx = (struct Sass_File_Context*) cptr;
+    ctx = sass_file_context_get_context(ctx_w->fctx);
   }
   else {
-    ctx = sass_data_context_get_context((struct Sass_Data_Context*) cptr);
     ctx_w->dctx = (struct Sass_Data_Context*) cptr;
+    ctx = sass_data_context_get_context(ctx_w->dctx);
   }
 
   struct Sass_Options* sass_options = sass_context_get_options(ctx);
@@ -92,9 +135,10 @@ void ExtractOptions(Local<Object> options, void* cptr, sass_context_wrapper* ctx
 
   Local<Function> importer_callback = Local<Function>::Cast(options->Get(NanNew("importer")));
 
-  ctx_w->importer_callback = new NanCallback(importer_callback);
+  ctx_w->importer_callback = NULL;
 
   if (!importer_callback->IsUndefined()) {
+    ctx_w->importer_callback = new NanCallback(importer_callback);
     uv_async_init(uv_default_loop(), &ctx_w->async, (uv_async_cb)dispatched_async_uv_callback);
     sass_option_set_importer(sass_options, sass_make_importer(sass_importer, ctx_w));
   }
@@ -114,6 +158,8 @@ void ExtractOptions(Local<Object> options, void* cptr, sass_context_wrapper* ctx
 }
 
 void GetStats(sass_context_wrapper* ctx_w, Sass_Context* ctx) {
+  NanScope();
+
   char** included_files = sass_context_get_included_files(ctx);
   Handle<Array> arr = NanNew<Array>();
 
@@ -123,13 +169,12 @@ void GetStats(sass_context_wrapper* ctx_w, Sass_Context* ctx) {
     }
   }
 
-  Local<Object> obj = NanNew(ctx_w->result);
-  obj->Get(NanNew("stats"))->ToObject()->Set(NanNew("includedFiles"), arr);
-
-  NanAssignPersistent(ctx_w->result, obj);
+  NanNew(ctx_w->result)->Get(NanNew("stats"))->ToObject()->Set(NanNew("includedFiles"), arr);
 }
 
 void GetSourceMap(sass_context_wrapper* ctx_w, Sass_Context* ctx) {
+  NanScope();
+
   Handle<Value> source_map;
 
   if (sass_context_get_error_status(ctx)) {
@@ -143,20 +188,16 @@ void GetSourceMap(sass_context_wrapper* ctx_w, Sass_Context* ctx) {
     source_map = NanNew<String>("{}");
   }
 
-  Local<Object> obj = NanNew(ctx_w->result);
-  obj->Set(NanNew("sourceMap"), source_map);
-
-  NanAssignPersistent(ctx_w->result, obj);
+  NanNew(ctx_w->result)->Set(NanNew("sourceMap"), source_map);
 }
 
 int GetResult(sass_context_wrapper* ctx_w, Sass_Context* ctx) {
+  NanScope();
+
   int status = sass_context_get_error_status(ctx);
 
   if (status == 0) {
-    Local<Object> obj = NanNew(ctx_w->result);
-    obj->Set(NanNew("css"), NanNew<String>(sass_context_get_output_string(ctx)));
-
-    NanAssignPersistent(ctx_w->result, obj);
+    NanNew(ctx_w->result)->Set(NanNew("css"), NanNew<String>(sass_context_get_output_string(ctx)));
 
     GetStats(ctx_w, ctx);
     GetSourceMap(ctx_w, ctx);
@@ -198,6 +239,10 @@ void make_callback(uv_work_t* req) {
     node::FatalException(try_catch);
   }
 
+  if (ctx_w->importer_callback) {
+    uv_close((uv_handle_t*)&ctx_w->async, NULL);
+  }
+
   sass_free_context_wrapper(ctx_w);
 }
 
@@ -228,6 +273,7 @@ NAN_METHOD(RenderSync) {
   sass_context_wrapper* ctx_w = sass_make_context_wrapper();
 
   ExtractOptions(options, dctx, ctx_w, false, true);
+
   compile_data(dctx);
 
   int result = GetResult(ctx_w, ctx);
@@ -238,7 +284,6 @@ NAN_METHOD(RenderSync) {
   }
 
   sass_free_context_wrapper(ctx_w);
-  free(source_string);
 
   if (result != 0) {
     NanThrowError(error);
@@ -260,7 +305,6 @@ NAN_METHOD(RenderFile) {
   int status = uv_queue_work(uv_default_loop(), &ctx_w->request, compile_it, (uv_after_work_cb)make_callback);
 
   assert(status == 0);
-  free(input_path);
 
   NanReturnUndefined();
 }
@@ -285,7 +329,6 @@ NAN_METHOD(RenderFileSync) {
   }
 
   sass_free_context_wrapper(ctx_w);
-  free(input_path);
 
   if (result != 0) {
     NanThrowError(error);
@@ -309,51 +352,13 @@ NAN_METHOD(ImportedCallback) {
 
   sass_context_wrapper* ctx_w = imports_collection[index];
 
-  if (returned_value->IsArray()) {
-    Handle<Array> array = Handle<Array>::Cast(returned_value);
-
-    ctx_w->imports = sass_make_import_list(array->Length());
-
-    for (size_t i = 0; i < array->Length(); ++i) {
-      Local<Value> value = array->Get(i);
-
-      if (!value->IsObject())
-        continue;
-
-      Local<Object> object = Local<Object>::Cast(value);
-      char* path = CreateString(object->Get(String::New("file")));
-      char* contents = CreateString(object->Get(String::New("contents")));
-
-      ctx_w->imports[i] = sass_make_import_entry(path, (!contents || contents[0] == '\0') ? 0 : strdup(contents), 0);
-    }
-  }
-  else if (returned_value->IsObject()) {
-    ctx_w->imports = sass_make_import_list(1);
-    Local<Object> object = Local<Object>::Cast(returned_value);
-    char* path = CreateString(object->Get(String::New("file")));
-    char* contents = CreateString(object->Get(String::New("contents")));
-
-    ctx_w->imports[0] = sass_make_import_entry(path, (!contents || contents[0] == '\0') ? 0 : strdup(contents), 0);
-  }
-  else {
-    ctx_w->imports = sass_make_import_list(1);
-    ctx_w->imports[0] = sass_make_import_entry(ctx_w->file, 0, 0);
-  }
-
+  prepare_import_results(returned_value, ctx_w);
   uv_cond_signal(&ctx_w->importer_condition_variable);
 
   if (try_catch.HasCaught()) {
     node::FatalException(try_catch);
   }
 
-  if (!ctx_w->success_callback) {
-    /*
-     *  that is sync: RenderSync() or RenderFileSync,
-     *  we ran it explictly, so we stop it similarly.
-     */
-    uv_stop(ctx_w->async.loop);
-  }
-
   NanReturnValue(NanNew<Number>(0));
 }
 

src/sass_context_wrapper.cpp

@@ -4,7 +4,7 @@ extern "C" {
   using namespace std;
 
   void compile_it(uv_work_t* req) {
-    sass_context_wrapper* ctx_w = static_cast<sass_context_wrapper*>(req->data);
+    sass_context_wrapper* ctx_w = (sass_context_wrapper*)req->data;
 
     if (ctx_w->dctx) {
       compile_data(ctx_w->dctx);
@@ -23,9 +23,10 @@ extern "C" {
   }
 
   sass_context_wrapper* sass_make_context_wrapper() {
-    auto ctx_w = (sass_context_wrapper*)calloc(1, sizeof(sass_context_wrapper));
+    sass_context_wrapper* ctx_w = (sass_context_wrapper*)calloc(1, sizeof(sass_context_wrapper));
     uv_mutex_init(&ctx_w->importer_mutex);
     uv_cond_init(&ctx_w->importer_condition_variable);
+
     return ctx_w;
   }
 

test/api.js

@@ -364,36 +364,6 @@ describe('api', function() {
   describe('.renderSync(importer)', function() {
     var src = read(fixture('include-files/index.scss'), 'utf8');
 
-    it('should override imports with "data" as input and fires callback with file and contents', function(done) {
-      var result = sass.renderSync({
-        data: src,
-        importer: function(url, prev, done) {
-          done({
-            file: '/some/other/path.scss',
-            contents: 'div {color: yellow;}'
-          });
-        }
-      });
-
-      assert.equal(result.css.trim(), 'div {\n  color: yellow; }\n\ndiv {\n  color: yellow; }');
-      done();
-    });
-
-    it('should override imports with "file" as input and fires callback with file and contents', function(done) {
-      var result = sass.renderSync({
-        file: fixture('include-files/index.scss'),
-        importer: function(url, prev, done) {
-          done({
-            file: '/some/other/path.scss',
-            contents: 'div {color: yellow;}'
-          });
-        }
-      });
-
-      assert.equal(result.css.trim(), 'div {\n  color: yellow; }\n\ndiv {\n  color: yellow; }');
-      done();
-    });
-
     it('should override imports with "data" as input and returns file and contents', function(done) {
       var result = sass.renderSync({
         data: src,
@@ -424,34 +394,6 @@ describe('api', function() {
       done();
     });
 
-    it('should override imports with "data" as input and fires callback with file', function(done) {
-      var result = sass.renderSync({
-        data: src,
-        importer: function(url, /* jshint unused:false */ prev, done) {
-          done({
-            file: path.resolve(path.dirname(fixture('include-files/index.scss')), url + (path.extname(url) ? '' : '.scss'))
-          });
-        }
-      });
-
-      assert.equal(result.css.trim(), '');
-      done();
-    });
-
-    it('should override imports with "file" as input and fires callback with file', function(done) {
-      var result = sass.renderSync({
-        file: fixture('include-files/index.scss'),
-        importer: function(url, prev, done) {
-          done({
-            file: path.resolve(path.dirname(prev), url + (path.extname(url) ? '' : '.scss'))
-          });
-        }
-      });
-
-      assert.equal(result.css.trim(), '');
-      done();
-    });
-
     it('should override imports with "data" as input and returns file', function(done) {
       var result = sass.renderSync({
         data: src,
@@ -480,34 +422,6 @@ describe('api', function() {
       done();
     });
 	
-    it('should override imports with "data" as input and fires callback with contents', function(done) {
-      var result = sass.renderSync({
-        data: src,
-        importer: function(url, prev, done) {
-          done({
-            contents: 'div {color: yellow;}'
-          });
-        }
-      });
-
-      assert.equal(result.css.trim(), 'div {\n  color: yellow; }\n\ndiv {\n  color: yellow; }');
-      done();
-    });
-
-    it('should override imports with "file" as input and fires callback with contents', function(done) {
-      var result = sass.renderSync({
-        file: fixture('include-files/index.scss'),
-        importer: function(url, prev, done) {
-          done({
-            contents: 'div {color: yellow;}'
-          });
-        }
-      });
-
-      assert.equal(result.css.trim(), 'div {\n  color: yellow; }\n\ndiv {\n  color: yellow; }');
-      done();
-    });
-
     it('should override imports with "data" as input and returns contents', function(done) {
       var result = sass.renderSync({
         data: src,