Importer: Throws error on invalid return type.

Issue URL: #670.
PR URL: #833.

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;
 };
 

test/api.js

@@ -260,7 +260,7 @@ describe('api', function() {
     it('should override imports with "data" as input and returns file', function(done) {
       sass.render({
         data: src,
-        importer: function(url, /* jshint unused:false */ prev) {
+        importer: function(url) {
           return {
             file: path.resolve(path.dirname(fixture('include-files/index.scss')), url + (path.extname(url) ? '' : '.scss'))
           };
@@ -414,7 +414,7 @@ describe('api', function() {
           done(new Error('doesn\'t exist!'));
         }
       }, function(error) {
-        assert.equal(error.message, 'doesn\'t exist!');
+        assert(/doesn\'t exist!/.test(error.message));
         done();
       });
     });
@@ -426,7 +426,19 @@ describe('api', function() {
           return new Error('doesn\'t exist!');
         }
       }, function(error) {
-        assert.equal(error.message, 'doesn\'t exist!');
+        assert(/doesn\'t exist!/.test(error.message));
+        done();
+      });
+    });
+
+    it('should throw exception when importer returns an invalid value', function(done) {
+      sass.render({
+        data: src,
+        importer: function() {
+          return { contents: new Buffer('i am not a string!') };
+        }
+      }, function(error) {
+        assert(/returned value of `contents` must be a string/.test(error.message));
         done();
       });
     });
@@ -851,144 +863,95 @@ describe('api', function() {
     });
   });
 
-  describe('.renderSync(functions)', function() {
-    it('should call custom function in sync mode', function(done) {
-      var result = sass.renderSync({
-        data: 'div { width: cos(0) * 50px; }',
-        functions: {
-          'cos($a)': function(angle) {
-            if (!(angle instanceof sass.types.Number)) {
-              throw new TypeError('Unexpected type for "angle"');
-            }
-            return new sass.types.Number(Math.cos(angle.getValue()));
-          }
-        }
-      });
+  describe('.render({stats: {}})', function() {
+    var start = Date.now();
 
-      assert.equal(result.css.toString().trim(), 'div {\n  width: 50px; }');
-      done();
+    it('should provide a start timestamp', function(done) {
+      sass.render({
+        file: fixture('include-files/index.scss')
+      }, function(error, result) {
+        assert(!error);
+        assert(typeof result.stats.start === 'number');
+        assert(result.stats.start >= start);
+        done();
+      });
     });
 
-    it('should return a list of selectors after calling the headings custom function', function(done) {
-      var result = sass.renderSync({
-        data: '#{headings(2,5)} { color: #08c; }',
-        functions: {
-          'headings($from: 0, $to: 6)': function(from, to) {
-            var i, f = from.getValue(), t = to.getValue(),
-                list = new sass.types.List(t - f + 1);
-
-            for (i = f; i <= t; i++) {
-              list.setValue(i - f, new sass.types.String('h' + i));
-            }
-
-            return list;
-          }
-        }
+    it('should provide an end timestamp', function(done) {
+      sass.render({
+        file: fixture('include-files/index.scss')
+      }, function(error, result) {
+        assert(!error);
+        assert(typeof result.stats.end === 'number');
+        assert(result.stats.end >= result.stats.start);
+        done();
       });
-
-      assert.equal(result.css.toString().trim(), 'h2, h3, h4, h5 {\n  color: #08c; }');
-      done();
     });
 
-    it('should let custom function invoke sass types constructors without the `new` keyword', function(done) {
-      var result = sass.renderSync({
-        data: 'div { color: foo(); }',
-        functions: {
-          'foo()': function() {
-            return sass.types.Number(42, 'em');
-          }
-        }
+    it('should provide a duration', function(done) {
+      sass.render({
+        file: fixture('include-files/index.scss')
+      }, function(error, result) {
+        assert(!error);
+        assert(typeof result.stats.duration === 'number');
+        assert.equal(result.stats.end - result.stats.start, result.stats.duration);
+        done();
       });
-
-      assert.equal(result.css.toString().trim(), 'div {\n  color: 42em; }');
-      done();
     });
 
-    it('should let us register custom functions without signatures', function(done) {
-      var result = sass.renderSync({
-        data: 'div { color: foo(20, 22); }',
-        functions: {
-          foo: function(a, b) {
-            return new sass.types.Number(a.getValue() + b.getValue(), 'em');
-          }
-        }
+    it('should contain the given entry file', function(done) {
+      sass.render({
+        file: fixture('include-files/index.scss')
+      }, function(error, result) {
+        assert(!error);
+        assert.equal(result.stats.entry, fixture('include-files/index.scss'));
+        done();
       });
-
-      assert.equal(result.css.toString().trim(), 'div {\n  color: 42em; }');
-      done();
     });
 
-    it('should fail when returning anything other than a sass value from a custom function', function(done) {
-      assert.throws(function() {
-        sass.renderSync({
-          data: 'div { color: foo(); }',
-          functions: {
-            'foo()': function() {
-              return {};
-            }
-          }
-        });
-      }, /A SassValue object was expected/);
+    it('should contain an array of all included files', function(done) {
+      var expected = [
+        fixture('include-files/bar.scss').replace(/\\/g, '/'),
+        fixture('include-files/foo.scss').replace(/\\/g, '/'),
+        fixture('include-files/index.scss').replace(/\\/g, '/')
+      ];
 
-      done();
+      sass.render({
+        file: fixture('include-files/index.scss')
+      }, function(error, result) {
+        assert(!error);
+        assert.deepEqual(result.stats.includedFiles, expected);
+        done();
+      });
     });
 
-    it('should properly bubble up standard JS errors thrown by custom functions', function(done) {
-      assert.throws(function() {
-        sass.renderSync({
-          data: 'div { color: foo(); }',
-          functions: {
-            'foo()': function() {
-              throw new RangeError('This is a test error');
-            }
-          }
-        });
-      }, /This is a test error/);
+    it('should contain array with the entry if there are no import statements', function(done) {
+      var expected = fixture('simple/index.scss').replace(/\\/g, '/');
 
-      done();
+      sass.render({
+        file: fixture('simple/index.scss')
+      }, function(error, result) {
+        assert.deepEqual(result.stats.includedFiles, [expected]);
+        done();
+      });
     });
 
-    it('should properly bubble up unknown errors thrown by custom functions', function(done) {
-      assert.throws(function() {
-        sass.renderSync({
-          data: 'div { color: foo(); }',
-          functions: {
-            'foo()': function() {
-              throw {};
-            }
-          }
-        });
-      }, /unexpected error/);
-
-      done();
+    it('should state `data` as entry file', function(done) {
+      sass.render({
+        data: read(fixture('simple/index.scss'), 'utf8')
+      }, function(error, result) {
+        assert.equal(result.stats.entry, 'data');
+        done();
+      });
     });
 
-    it('should properly bubble up errors from sass value getters/setters/constructors', function(done) {
-      assert.throws(function() {
-        sass.renderSync({
-          data: 'div { color: foo(); }',
-          functions: {
-            'foo()': function() {
-              return sass.types.Boolean('foo');
-            }
-          }
-        });
-      }, /Expected one boolean argument/);
-
-      assert.throws(function() {
-        sass.renderSync({
-          data: 'div { color: foo(); }',
-          functions: {
-            'foo()': function() {
-              var ret = new sass.types.Number(42);
-              ret.setUnit(123);
-              return ret;
-            }
-          }
-        });
-      }, /Supplied value should be a string/);
-
-      done();
+    it('should contain an empty array as includedFiles', function(done) {
+      sass.render({
+        data: read(fixture('simple/index.scss'), 'utf8')
+      }, function(error, result) {
+        assert.deepEqual(result.stats.includedFiles, []);
+        done();
+      });
     });
   });
 
@@ -1112,7 +1075,7 @@ describe('api', function() {
     it('should override imports with "data" as input and returns file', function(done) {
       var result = sass.renderSync({
         data: src,
-        importer: function(url, /* jshint unused:false */ prev) {
+        importer: function(url) {
           return {
             file: path.resolve(path.dirname(fixture('include-files/index.scss')), url + (path.extname(url) ? '' : '.scss'))
           };
@@ -1211,97 +1174,159 @@ describe('api', function() {
 
       done();
     });
-  });
 
-  describe('.render({stats: {}})', function() {
-    var start = Date.now();
+    it('should throw exception when importer returns an invalid value', function(done) {
+      assert.throws(function() {
+        sass.renderSync({
+          data: src,
+          importer: function() {
+            return { contents: new Buffer('i am not a string!') };
+          }
+        });
+      }, /returned value of `contents` must be a string/);
 
-    it('should provide a start timestamp', function(done) {
-      sass.render({
-        file: fixture('include-files/index.scss')
-      }, function(error, result) {
-        assert(!error);
-        assert(typeof result.stats.start === 'number');
-        assert(result.stats.start >= start);
-        done();
+      done();
+    });
+  });
+
+  describe('.renderSync(functions)', function() {
+    it('should call custom function in sync mode', function(done) {
+      var result = sass.renderSync({
+        data: 'div { width: cos(0) * 50px; }',
+        functions: {
+          'cos($a)': function(angle) {
+            if (!(angle instanceof sass.types.Number)) {
+              throw new TypeError('Unexpected type for "angle"');
+            }
+            return new sass.types.Number(Math.cos(angle.getValue()));
+          }
+        }
       });
+
+      assert.equal(result.css.toString().trim(), 'div {\n  width: 50px; }');
+      done();
     });
 
-    it('should provide an end timestamp', function(done) {
-      sass.render({
-        file: fixture('include-files/index.scss')
-      }, function(error, result) {
-        assert(!error);
-        assert(typeof result.stats.end === 'number');
-        assert(result.stats.end >= result.stats.start);
-        done();
+    it('should return a list of selectors after calling the headings custom function', function(done) {
+      var result = sass.renderSync({
+        data: '#{headings(2,5)} { color: #08c; }',
+        functions: {
+          'headings($from: 0, $to: 6)': function(from, to) {
+            var i, f = from.getValue(), t = to.getValue(),
+                list = new sass.types.List(t - f + 1);
+
+            for (i = f; i <= t; i++) {
+              list.setValue(i - f, new sass.types.String('h' + i));
+            }
+
+            return list;
+          }
+        }
       });
+
+      assert.equal(result.css.toString().trim(), 'h2, h3, h4, h5 {\n  color: #08c; }');
+      done();
     });
 
-    it('should provide a duration', function(done) {
-      sass.render({
-        file: fixture('include-files/index.scss')
-      }, function(error, result) {
-        assert(!error);
-        assert(typeof result.stats.duration === 'number');
-        assert.equal(result.stats.end - result.stats.start, result.stats.duration);
-        done();
+    it('should let custom function invoke sass types constructors without the `new` keyword', function(done) {
+      var result = sass.renderSync({
+        data: 'div { color: foo(); }',
+        functions: {
+          'foo()': function() {
+            return sass.types.Number(42, 'em');
+          }
+        }
       });
+
+      assert.equal(result.css.toString().trim(), 'div {\n  color: 42em; }');
+      done();
     });
 
-    it('should contain the given entry file', function(done) {
-      sass.render({
-        file: fixture('include-files/index.scss')
-      }, function(error, result) {
-        assert(!error);
-        assert.equal(result.stats.entry, fixture('include-files/index.scss'));
-        done();
+    it('should let us register custom functions without signatures', function(done) {
+      var result = sass.renderSync({
+        data: 'div { color: foo(20, 22); }',
+        functions: {
+          foo: function(a, b) {
+            return new sass.types.Number(a.getValue() + b.getValue(), 'em');
+          }
+        }
       });
+
+      assert.equal(result.css.toString().trim(), 'div {\n  color: 42em; }');
+      done();
     });
 
-    it('should contain an array of all included files', function(done) {
-      var expected = [
-        fixture('include-files/bar.scss').replace(/\\/g, '/'),
-        fixture('include-files/foo.scss').replace(/\\/g, '/'),
-        fixture('include-files/index.scss').replace(/\\/g, '/')
-      ];
+    it('should fail when returning anything other than a sass value from a custom function', function(done) {
+      assert.throws(function() {
+        sass.renderSync({
+          data: 'div { color: foo(); }',
+          functions: {
+            'foo()': function() {
+              return {};
+            }
+          }
+        });
+      }, /A SassValue object was expected/);
 
-      sass.render({
-        file: fixture('include-files/index.scss')
-      }, function(error, result) {
-        assert(!error);
-        assert.deepEqual(result.stats.includedFiles, expected);
-        done();
-      });
+      done();
     });
 
-    it('should contain array with the entry if there are no import statements', function(done) {
-      var expected = fixture('simple/index.scss').replace(/\\/g, '/');
+    it('should properly bubble up standard JS errors thrown by custom functions', function(done) {
+      assert.throws(function() {
+        sass.renderSync({
+          data: 'div { color: foo(); }',
+          functions: {
+            'foo()': function() {
+              throw new RangeError('This is a test error');
+            }
+          }
+        });
+      }, /This is a test error/);
 
-      sass.render({
-        file: fixture('simple/index.scss')
-      }, function(error, result) {
-        assert.deepEqual(result.stats.includedFiles, [expected]);
-        done();
-      });
+      done();
     });
 
-    it('should state `data` as entry file', function(done) {
-      sass.render({
-        data: read(fixture('simple/index.scss'), 'utf8')
-      }, function(error, result) {
-        assert.equal(result.stats.entry, 'data');
-        done();
-      });
+    it('should properly bubble up unknown errors thrown by custom functions', function(done) {
+      assert.throws(function() {
+        sass.renderSync({
+          data: 'div { color: foo(); }',
+          functions: {
+            'foo()': function() {
+              throw {};
+            }
+          }
+        });
+      }, /unexpected error/);
+
+      done();
     });
 
-    it('should contain an empty array as includedFiles', function(done) {
-      sass.render({
-        data: read(fixture('simple/index.scss'), 'utf8')
-      }, function(error, result) {
-        assert.deepEqual(result.stats.includedFiles, []);
-        done();
-      });
+    it('should properly bubble up errors from sass value getters/setters/constructors', function(done) {
+      assert.throws(function() {
+        sass.renderSync({
+          data: 'div { color: foo(); }',
+          functions: {
+            'foo()': function() {
+              return sass.types.Boolean('foo');
+            }
+          }
+        });
+      }, /Expected one boolean argument/);
+
+      assert.throws(function() {
+        sass.renderSync({
+          data: 'div { color: foo(); }',
+          functions: {
+            'foo()': function() {
+              var ret = new sass.types.Number(42);
+              ret.setUnit(123);
+              return ret;
+            }
+          }
+        });
+      }, /Supplied value should be a string/);
+
+      done();
     });
   });
 
@@ -1377,7 +1402,7 @@ describe('api', function() {
     });
   });
 
-  describe('.info()', function() {
+  describe('.info', function() {
     var package = require('../package.json'),
         info = sass.info;