Don't try to load absolute URLs from the base importer (#2077)

Author: nex3

CHANGELOG.md

@@ -1,4 +1,10 @@
-## 1.66.2
+## 1.67.0
+
+* **Potentially breaking bug fix**: The importer used to load a given file is no
+  longer used to load absolute URLs that appear in that file. This was
+  unintented behavior that contradicted the Sass specification. Absolute URLs
+  will now correctly be loaded only from the global importer list. This applies
+  to the modern JS API, the Dart API, and the embedded protocol.
 
 ### Embedded Sass
 

lib/src/async_import_cache.dart

@@ -142,7 +142,7 @@ final class AsyncImportCache {
       throw "Custom importers are required to load stylesheets when compiling in the browser.";
     }
 
-    if (baseImporter != null) {
+    if (baseImporter != null && url.scheme == '') {
       var relativeResult = await putIfAbsentAsync(_relativeCanonicalizeCache, (
         url,
         forImport: forImport,

lib/src/import_cache.dart

@@ -5,7 +5,7 @@
 // DO NOT EDIT. This file was generated from async_import_cache.dart.
 // See tool/grind/synchronize.dart for details.
 //
-// Checksum: 1b6289e0dd362fcb02f331a16a30fe94050b4e17
+// Checksum: 3e4cae79c03ce2af6626b1822f1468523b401e90
 //
 // ignore_for_file: unused_import
 
@@ -142,7 +142,7 @@ final class ImportCache {
       throw "Custom importers are required to load stylesheets when compiling in the browser.";
     }
 
-    if (baseImporter != null) {
+    if (baseImporter != null && url.scheme == '') {
       var relativeResult = _relativeCanonicalizeCache.putIfAbsent((
         url,
         forImport: forImport,

pubspec.yaml

@@ -1,5 +1,5 @@
 name: sass
-version: 1.66.2-dev
+version: 1.67.0-dev
 description: A Sass implementation in Dart.
 homepage: https://github.com/sass/dart-sass
 

test/dart_api/importer_test.dart

@@ -201,6 +201,61 @@ void main() {
     })));
   });
 
+  group("compileString()'s importer option", () {
+    test("loads relative imports from the entrypoint", () {
+      var css = compileString('@import "orange";',
+          importer: TestImporter((url) => Uri.parse("u:$url"), (url) {
+            var color = url.path;
+            return ImporterResult('.$color {color: $color}', indented: false);
+          }));
+
+      expect(css, equals(".orange {\n  color: orange;\n}"));
+    });
+
+    test("loads imports relative to the entrypoint's URL", () {
+      var css = compileString('@import "baz/qux";',
+          importer: TestImporter((url) => url.resolve("bang"), (url) {
+            return ImporterResult('a {result: "${url.path}"}', indented: false);
+          }),
+          url: Uri.parse("u:foo/bar"));
+
+      expect(css, equals('a {\n  result: "foo/baz/bang";\n}'));
+    });
+
+    test("doesn't load absolute imports", () {
+      var css = compileString('@import "u:orange";',
+          importer: TestImporter((_) => throw "Should not be called",
+              (_) => throw "Should not be called"),
+          importers: [
+            TestImporter((url) => url, (url) {
+              var color = url.path;
+              return ImporterResult('.$color {color: $color}', indented: false);
+            })
+          ]);
+
+      expect(css, equals(".orange {\n  color: orange;\n}"));
+    });
+
+    test("doesn't load from other importers", () {
+      var css = compileString('@import "u:midstream";',
+          importer: TestImporter((_) => throw "Should not be called",
+              (_) => throw "Should not be called"),
+          importers: [
+            TestImporter((url) => url, (url) {
+              if (url.path == "midstream") {
+                return ImporterResult("@import 'orange';", indented: false);
+              } else {
+                var color = url.path;
+                return ImporterResult('.$color {color: $color}',
+                    indented: false);
+              }
+            })
+          ]);
+
+      expect(css, equals(".orange {\n  color: orange;\n}"));
+    });
+  });
+
   group("currentLoadFromImport is", () {
     test("true from an @import", () {
       compileString('@import "foo"', importers: [FromImportImporter(true)]);

test/dart_api_test.dart

@@ -12,6 +12,8 @@ import 'package:test_descriptor/test_descriptor.dart' as d;
 import 'package:sass/sass.dart';
 import 'package:sass/src/exception.dart';
 
+import 'dart_api/test_importer.dart';
+
 void main() {
   // TODO(nweiz): test SASS_PATH when dart-lang/sdk#28160 is fixed.
 
@@ -139,8 +141,9 @@ void main() {
       expect(css, equals("a {\n  b: from-relative;\n}"));
     });
 
-    test("the original importer takes precedence over other importers",
-        () async {
+    test(
+        "the original importer takes precedence over other importers for "
+        "relative imports", () async {
       await d.dir(
           "original", [d.file("other.scss", "a {b: from-original}")]).create();
       await d
@@ -153,6 +156,21 @@ void main() {
       expect(css, equals("a {\n  b: from-original;\n}"));
     });
 
+    test("importer order is preserved for absolute imports", () {
+      var css = compileString('@import "second:other";', importers: [
+        TestImporter((url) => url.scheme == 'first' ? url : null,
+            (url) => ImporterResult('a {from: first}', indented: false)),
+        // This importer should only be invoked once, because when the
+        // "first:other" import is resolved it should be passed to the first
+        // importer first despite being in the second importer's file.
+        TestImporter(
+            expectAsync1((url) => url.scheme == 'second' ? url : null,
+                count: 1),
+            (url) => ImporterResult('@import "first:other";', indented: false)),
+      ]);
+      expect(css, equals("a {\n  from: first;\n}"));
+    });
+
     test("importers take precedence over load paths", () async {
       await d.dir("load-path",
           [d.file("other.scss", "a {b: from-load-path}")]).create();