Merge pull request #991 from sass/node-importer-file-and-contents

Don't read `file` if Node importer returns `file` and `contents`.
sass · Apr 29, 2020 · 39358fb · 39358fb
2 parents 7160b64 + 21a1c58
commit 39358fb
Show file tree

Hide file tree

Showing 3 changed files with 38 additions and 13 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -5,6 +5,10 @@
 * Add `sass.NULL`, `sass.TRUE`, and `sass.FALSE` constants to match Node Sass's
   API.
 
+* If a custom Node importer returns both `file` and `contents`, don't attempt to
+  read the `file`. Instead, use the `contents` provided by the importer, with
+  `file` as the canonical url.
+
 ## 1.26.5
 
 * No user-visible changes.

diff --git a/lib/src/importer/node/implementation.dart b/lib/src/importer/node/implementation.dart
@@ -182,14 +182,15 @@ class NodeImporter {
     if (value is! NodeImporterResult) return null;
 
     var result = value as NodeImporterResult;
-    if (result.file != null) {
+    if (result.file == null) {
+      return Tuple2(result.contents ?? '', url);
+    } else if (result.contents != null) {
+      return Tuple2(result.contents, result.file);
+    } else {
       var resolved = _resolveRelativePath(result.file, previous, forImport) ??
           _resolveLoadPath(result.file, previous, forImport);
       if (resolved != null) return resolved;
-
       throw "Can't find stylesheet to import.";
-    } else {
-      return Tuple2(result.contents ?? '', url);
     }
   }
 

diff --git a/test/node_api/importer_test.dart b/test/node_api/importer_test.dart
@@ -30,15 +30,6 @@ void main() {
     await writeTextFile(sassPath, 'a {b: c}');
   });
 
-  test("can import a file by contents", () {
-    expect(
-        renderSync(RenderOptions(
-            data: "@import 'foo'",
-            importer: allowInterop((void _, void __) =>
-                NodeImporterResult(contents: 'a {b: c}')))),
-        equalsIgnoringWhitespace('a { b: c; }'));
-  });
-
   test("imports cascade through importers", () {
     expect(
         renderSync(RenderOptions(data: "@import 'foo'", importer: [
@@ -121,6 +112,35 @@ void main() {
     });
   });
 
+  group("with contents", () {
+    test("imports a file by contents", () {
+      expect(
+          renderSync(RenderOptions(
+              data: "@import 'foo'",
+              importer: allowInterop((void _, void __) =>
+                  NodeImporterResult(contents: 'a {b: c}')))),
+          equalsIgnoringWhitespace('a { b: c; }'));
+    });
+
+    test("contents take precedence over file name", () {
+      expect(
+          renderSync(RenderOptions(
+              data: "@import 'foo'",
+              importer: allowInterop((void _, void __) =>
+                  NodeImporterResult(contents: 'x {y: z}', file: sassPath)))),
+          equalsIgnoringWhitespace('x { y: z; }'));
+    });
+
+    test("contents use file name as canonical url", () {
+      var result = sass.renderSync(RenderOptions(
+          data: "@import 'foo'",
+          importer: allowInterop(expectAsync2((void _, void __) {
+            return NodeImporterResult(contents: '', file: 'bar');
+          }))));
+      expect(result.stats.includedFiles, equals(['bar']));
+    });
+  });
+
   group("with a file redirect", () {
     test("imports the chosen file", () {
       expect(