Merge pull request #833 from am11/issue-670

Importer: Throws error on invalid return type

README.md

@@ -89,7 +89,7 @@ When returning or calling `done()` with `{ contents: "String" }`, the string val
 
 Starting from v3.0.0, `this` refers to a contextual scope for the immediate run of `sass.render` or `sass.renderSync`
 
-Starting from v3.0.0, importer can be an array of functions, which will be called by libsass in the order of their occurance in array. This helps user specify special importer for particular kind of path (filesystem, http). If the importer does not handle particular path, it should return `sass.NULL`. See [functions section](#functions) for more details on Sass types.
+Starting from v3.0.0, importer can be an array of functions, which will be called by libsass in the order of their occurance in array. This helps user specify special importer for particular kind of path (filesystem, http). If an importer does not want to handle a particular path, it should return `sass.NULL`. See [functions section](#functions--v300) for more details on Sass types.
 
 ### functions (>= v3.0.0)
 `functions` is an `Object` that holds a collection of custom functions that may be invoked by the sass files being compiled. They may take zero or more input parameters and must return a value either synchronously (`return ...;`) or asynchronously (`done();`). Those parameters will be instances of one of the constructors contained in the `require('node-sass').types` hash. The return value must be of one of these types as well. See the list of available types below:
@@ -431,7 +431,7 @@ The interface for command-line usage is fairly simplistic at this stage, as seen
 Output will be saved with the same name as input SASS file into the current working directory if it's omitted.
 
 ### Usage
- `node-sass [options] <input.scss> [output.css]`  
+ `node-sass [options] <input.scss> [output.css]`
  `cat <input.scss> | node-sass > output.css`
 
  **Options:**

src/custom_importer_bridge.cpp

@@ -17,7 +17,7 @@ SassImportList CustomImporterBridge::post_process_return_value(Handle<Value> val
       Local<Value> value = array->Get(static_cast<uint32_t>(i));
 
       if (!value->IsObject()) {
-        continue;
+        NanThrowError(NanNew("returned array must only contain object literals"));
       }
 
       Local<Object> object = Local<Object>::Cast(value);
@@ -30,11 +30,7 @@ SassImportList CustomImporterBridge::post_process_return_value(Handle<Value> val
         sass_import_set_error(imports[i], message, -1, -1);
       }
       else {
-        char* path = create_string(object->Get(NanNew<String>("file")));
-        char* contents = create_string(object->Get(NanNew<String>("contents")));
-        char* srcmap = create_string(object->Get(NanNew<String>("map")));
-
-        imports[i] = sass_make_import_entry(path, contents, srcmap);
+        imports[i] = get_importer_entry(object);
       }
     }
   }
@@ -49,17 +45,44 @@ SassImportList CustomImporterBridge::post_process_return_value(Handle<Value> val
   }
   else if (returned_value->IsObject()) {
     imports = sass_make_import_list(1);
-    Local<Object> object = Local<Object>::Cast(returned_value);
-    char* path = create_string(object->Get(NanNew<String>("file")));
-    char* contents = create_string(object->Get(NanNew<String>("contents")));
-    char* srcmap = create_string(object->Get(NanNew<String>("map")));
-
-    imports[0] = sass_make_import_entry(path, contents, srcmap);
+    imports[0] = get_importer_entry(Local<Object>::Cast(returned_value));
   }
 
   return imports;
 }
 
+Sass_Import* CustomImporterBridge::get_importer_entry(const Local<Object>& object) const {
+  auto returned_file = object->Get(NanNew<String>("file"));
+
+  if (!returned_file->IsUndefined() && !returned_file->IsString()) {
+    auto entry = sass_make_import_entry(0, 0, 0);
+    sass_import_set_error(entry, "returned value of `file` must be a string", -1, -1);
+    return entry;
+  }
+
+  auto returned_contents = object->Get(NanNew<String>("contents"));
+
+  if (!returned_contents->IsUndefined() && !returned_contents->IsString()) {
+    auto entry = sass_make_import_entry(0, 0, 0);
+    sass_import_set_error(entry, "returned value of `contents` must be a string", -1, -1);
+    return entry;
+  }
+
+  auto returned_map = object->Get(NanNew<String>("map"));
+
+  if (!returned_map->IsUndefined() && !returned_map->IsString()) {
+    auto entry = sass_make_import_entry(0, 0, 0);
+    sass_import_set_error(entry, "returned value of `map` must be a string", -1, -1);
+    return entry;
+  }
+
+  char* path = create_string(returned_file);
+  char* contents = create_string(returned_contents);
+  char* srcmap = create_string(returned_map);
+
+  return sass_make_import_entry(path, contents, srcmap);
+}
+
 std::vector<Handle<Value>> CustomImporterBridge::pre_process_args(std::vector<void*> in) const {
   std::vector<Handle<Value>> out;
 

src/custom_importer_bridge.h

@@ -15,6 +15,7 @@ class CustomImporterBridge : public CallbackBridge<SassImportList> {
 
   private:
     SassImportList post_process_return_value(Handle<Value>) const;
+    Sass_Import* get_importer_entry(const Local<Object>&) const;
     std::vector<Handle<Value>> pre_process_args(std::vector<void*>) const;
 };