Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 4 commits into from
Sep 8, 2023
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 7 additions & 1 deletion CHANGELOG.md
Original file line number Diff line number Diff line change
@@ -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

Expand Down
2 changes: 1 addition & 1 deletion lib/src/async_import_cache.dart
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down
4 changes: 2 additions & 2 deletions lib/src/import_cache.dart
Original file line number Diff line number Diff line change
Expand Up @@ -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

Expand Down Expand Up @@ -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,
Expand Down
55 changes: 55 additions & 0 deletions test/dart_api/importer_test.dart
Original file line number Diff line number Diff line change
Expand Up @@ -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)]);
Expand Down
22 changes: 20 additions & 2 deletions test/dart_api_test.dart
Original file line number Diff line number Diff line change
Expand Up @@ -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.

Expand Down Expand Up @@ -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
Expand All @@ -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();
Expand Down
Loading