Merge pull request #268 from sass/node-api

Fix some Node API corner-cases

CHANGELOG.md

@@ -4,6 +4,15 @@
 
 [#260]: https://github.com/sass/dart-sass/issues/260
 
+### Node API
+
+* Errors are now subtypes of the `Error` type.
+
+* Allow both the `data` and `file` options to be passed to `render()` and
+  `renderSync()` at once. The `data` option will be used as the contents of the
+  stylesheet, and the `file` option will be used as the path for error reporting
+  and relative imports. This matches Node Sass's behavior.
+
 ## 1.0.0-rc.1
 
 * Add support for importing an `_index.scss` or `_index.sass` file when

lib/src/node.dart

@@ -3,6 +3,7 @@
 // https://opensource.org/licenses/MIT.
 
 import 'dart:async';
+import 'dart:js_util';
 
 import 'package:collection/collection.dart';
 import 'package:js/js.dart';
@@ -14,10 +15,10 @@ import 'compile.dart';
 import 'exception.dart';
 import 'executable.dart' as executable;
 import 'importer/node.dart';
+import 'node/error.dart';
 import 'node/exports.dart';
 import 'node/render_context.dart';
 import 'node/render_context_options.dart';
-import 'node/render_error.dart';
 import 'node/render_options.dart';
 import 'node/render_result.dart';
 import 'node/types.dart';
@@ -61,14 +62,14 @@ void main() {
 /// possible.
 ///
 /// [render]: https://github.com/sass/node-sass#options
-void _render(RenderOptions options,
-    void callback(RenderError error, RenderResult result)) {
+void _render(
+    RenderOptions options, void callback(JSError error, RenderResult result)) {
   if (options.fiber != null) {
     options.fiber.call(allowInterop(() {
       try {
         callback(null, _renderSync(options));
       } catch (error) {
-        callback(error as RenderError, null);
+        callback(error as JSError, null);
       }
     })).run();
   } else {
@@ -78,7 +79,7 @@ void _render(RenderOptions options,
       if (error is SassException) {
         callback(_wrapException(error), null);
       } else {
-        callback(newRenderError(error.toString(), status: 3), null);
+        callback(_newRenderError(error.toString(), status: 3), null);
       }
     });
   }
@@ -89,11 +90,6 @@ Future<RenderResult> _renderAsync(RenderOptions options) async {
   var start = new DateTime.now();
   CompileResult result;
   if (options.data != null) {
-    if (options.file != null) {
-      throw new ArgumentError(
-          "options.data and options.file may not both be set.");
-    }
-
     result = await compileStringAsync(options.data,
         nodeImporter: _parseImporter(options, start),
         functions: _parseFunctions(options, asynch: true),
@@ -102,7 +98,7 @@ Future<RenderResult> _renderAsync(RenderOptions options) async {
         useSpaces: options.indentType != 'tab',
         indentWidth: _parseIndentWidth(options.indentWidth),
         lineFeed: _parseLineFeed(options.linefeed),
-        url: 'stdin');
+        url: options.file == null ? 'stdin' : p.toUri(options.file).toString());
   } else if (options.file != null) {
     result = await compileAsync(options.file,
         nodeImporter: _parseImporter(options, start),
@@ -136,11 +132,6 @@ RenderResult _renderSync(RenderOptions options) {
     var start = new DateTime.now();
     CompileResult result;
     if (options.data != null) {
-      if (options.file != null) {
-        throw new ArgumentError(
-            "options.data and options.file may not both be set.");
-      }
-
       result = compileString(options.data,
           nodeImporter: _parseImporter(options, start),
           functions: DelegatingList.typed(_parseFunctions(options)),
@@ -149,7 +140,9 @@ RenderResult _renderSync(RenderOptions options) {
           useSpaces: options.indentType != 'tab',
           indentWidth: _parseIndentWidth(options.indentWidth),
           lineFeed: _parseLineFeed(options.linefeed),
-          url: 'stdin');
+          url: options.file == null
+              ? 'stdin'
+              : p.toUri(options.file).toString());
     } else if (options.file != null) {
       result = compile(options.file,
           nodeImporter: _parseImporter(options, start),
@@ -174,33 +167,37 @@ RenderResult _renderSync(RenderOptions options) {
   } on SassException catch (error) {
     jsThrow(_wrapException(error));
   } catch (error) {
-    jsThrow(newRenderError(error.toString(), status: 3));
+    jsThrow(_newRenderError(error.toString(), status: 3));
   }
   throw "unreachable";
 }
 
-/// Converts a [SassException] to a [RenderError].
-RenderError _wrapException(SassException exception) {
-  var trace = exception is SassRuntimeException
-      ? "\n" +
-          exception.trace
-              .toString()
-              .trimRight()
-              .split("\n")
-              .map((frame) => "  $frame")
-              .join("\n")
-      : "\n  ${p.prettyUri(exception.span.sourceUrl ?? '-')} "
-      "${exception.span.start.line + 1}:${exception.span.start.column + 1}  "
-      "root stylesheet";
+/// Converts an exception to a [JSError].
+JSError _wrapException(exception) {
+  if (exception is SassException) {
+    var trace = exception is SassRuntimeException
+        ? "\n" +
+            exception.trace
+                .toString()
+                .trimRight()
+                .split("\n")
+                .map((frame) => "  $frame")
+                .join("\n")
+        : "\n  ${p.prettyUri(exception.span.sourceUrl ?? '-')} "
+        "${exception.span.start.line + 1}:${exception.span.start.column + 1}  "
+        "root stylesheet";
 
-  return newRenderError(exception.message + trace,
-      formatted: exception.toString(),
-      line: exception.span.start.line + 1,
-      column: exception.span.start.column + 1,
-      file: exception.span.sourceUrl == null
-          ? 'stdin'
-          : p.fromUri(exception.span.sourceUrl),
-      status: 1);
+    return _newRenderError(exception.message + trace,
+        formatted: exception.toString(),
+        line: exception.span.start.line + 1,
+        column: exception.span.start.column + 1,
+        file: exception.span.sourceUrl == null
+            ? 'stdin'
+            : p.fromUri(exception.span.sourceUrl),
+        status: 1);
+  } else {
+    return new JSError(exception.toString());
+  }
 }
 
 /// Parses `functions` from [RenderOptions] into a list of [Callable]s or
@@ -335,3 +332,16 @@ LineFeed _parseLineFeed(String str) {
       return LineFeed.lf;
   }
 }
+
+/// Creates a [JSError] with the given fields added to it so it acts like a Node
+/// Sass error.
+JSError _newRenderError(String message,
+    {String formatted, int line, int column, String file, int status}) {
+  var error = new JSError(message);
+  if (formatted != null) setProperty(error, 'formatted', formatted);
+  if (line != null) setProperty(error, 'line', line);
+  if (column != null) setProperty(error, 'column', column);
+  if (file != null) setProperty(error, 'file', file);
+  if (status != null) setProperty(error, 'status', status);
+  return error;
+}

lib/src/node/error.dart

@@ -0,0 +1,11 @@
+// Copyright 2016 Google Inc. Use of this source code is governed by an
+// MIT-style license that can be found in the LICENSE file or at
+// https://opensource.org/licenses/MIT.
+
+import 'package:js/js.dart';
+
+@JS("Error")
+class JSError {
+  external String get message;
+  external JSError(String message);
+}

lib/src/node/render_error.dart

@@ -1,39 +0,0 @@
-// Copyright 2016 Google Inc. Use of this source code is governed by an
-// MIT-style license that can be found in the LICENSE file or at
-// https://opensource.org/licenses/MIT.
-
-import 'package:js/js.dart';
-
-import 'utils.dart';
-
-@JS()
-@anonymous
-class RenderError {
-  external String get message;
-  external String get formatted;
-  external int get line;
-  external int get column;
-  external String get file;
-  external int get status;
-
-  external factory RenderError._(
-      {String message,
-      String formatted,
-      int line,
-      int column,
-      String file,
-      int status});
-}
-
-RenderError newRenderError(String message,
-    {String formatted, int line, int column, String file, int status}) {
-  var error = new RenderError._(
-      message: message,
-      formatted: formatted,
-      line: line,
-      column: column,
-      file: file,
-      status: status);
-  setToString(error, () => "Error: $message");
-  return error;
-}

test/node_api/api.dart

@@ -7,16 +7,15 @@
 /// will be used in the real world without having to manually write any JS.
 
 import 'package:sass/src/node/fiber.dart';
-import 'package:sass/src/node/render_error.dart';
 import 'package:sass/src/node/render_options.dart';
 import 'package:sass/src/node/render_result.dart';
 import 'package:sass/src/util/path.dart';
 
 import 'package:js/js.dart';
 
+export 'package:sass/src/node/error.dart';
 export 'package:sass/src/node/importer_result.dart';
 export 'package:sass/src/node/render_context.dart';
-export 'package:sass/src/node/render_error.dart';
 export 'package:sass/src/node/render_options.dart';
 export 'package:sass/src/node/render_result.dart';
 
@@ -58,9 +57,14 @@ class Sass {
   external SassTypes get types;
 }
 
-@JS("Error")
-class JSError {
-  external JSError(String message);
+@JS()
+class RenderError {
+  external String get message;
+  external String get formatted;
+  external int get line;
+  external int get column;
+  external String get file;
+  external int get status;
 }
 
 @JS()

test/node_api_test.dart

@@ -10,6 +10,7 @@ import 'package:path/path.dart' as unsafePath;
 import 'package:test/test.dart';
 
 import 'package:sass/src/util/path.dart';
+import 'package:sass/src/node/utils.dart';
 
 import 'ensure_npm_package.dart';
 import 'hybrid.dart';
@@ -69,13 +70,6 @@ a {
 }'''));
     });
 
-    test("data and file may not both be set", () {
-      var error =
-          renderSyncError(new RenderOptions(data: "x {y: z}", file: sassPath));
-      expect(error.toString(),
-          contains('options.data and options.file may not both be set.'));
-    });
-
     test("one of data and file must be set", () {
       var error = renderSyncError(new RenderOptions());
       expect(error.toString(),
@@ -186,6 +180,37 @@ a {
       });
     });
 
+    group("with both data and file", () {
+      test("uses the data parameter as the source", () {
+        expect(renderSync(new RenderOptions(data: "x {y: z}", file: sassPath)),
+            equalsIgnoringWhitespace('x { y: z; }'));
+      });
+
+      test("doesn't require the file path to exist", () {
+        expect(
+            renderSync(new RenderOptions(
+                data: "x {y: z}", file: p.join(sandbox, 'non-existent.scss'))),
+            equalsIgnoringWhitespace('x { y: z; }'));
+      });
+
+      test("imports relative to the file path", () async {
+        await writeTextFile(p.join(sandbox, 'importee.scss'), 'x {y: z}');
+        expect(
+            renderSync(
+                new RenderOptions(data: "@import 'importee'", file: sassPath)),
+            equalsIgnoringWhitespace('x { y: z; }'));
+      });
+
+      test("reports errors from the file path", () {
+        var error =
+            renderSyncError(new RenderOptions(data: "x {y: }", file: sassPath));
+        expect(
+            error.toString(),
+            equals("Error: Expected expression.\n"
+                "  $sassPath 1:7  root stylesheet"));
+      });
+    });
+
     group("the result object", () {
       test("includes the filename", () {
         var result = sass.renderSync(new RenderOptions(file: sassPath));
@@ -245,6 +270,10 @@ a {
           error = renderSyncError(new RenderOptions(file: sassPath));
         });
 
+        test("is a JS Error", () async {
+          expect(isJSError(error), isTrue);
+        });
+
         test("has a useful toString() and message", () async {
           expect(
               error,
@@ -273,6 +302,10 @@ a {
           error = renderSyncError(new RenderOptions(data: "a {b: }"));
         });
 
+        test("is a JS Error", () async {
+          expect(isJSError(error), isTrue);
+        });
+
         test("has a useful toString() and message", () {
           expect(
               error,