From f02215bc7006d5282261c9c39cf141a82a58e9fb Mon Sep 17 00:00:00 2001 From: Natalie Weizenbaum Date: Thu, 14 Sep 2023 18:12:23 -0700 Subject: [PATCH] Expose the containing URL to importers under some circumstances Closes #3247 --- lib/src/async_import_cache.dart | 76 +++++++++++++-------- lib/src/import_cache.dart | 74 ++++++++++++-------- lib/src/importer.dart | 2 + lib/src/importer/async.dart | 27 ++++++++ lib/src/importer/js_to_dart/async.dart | 15 ++++- lib/src/importer/js_to_dart/file.dart | 2 + lib/src/importer/js_to_dart/sync.dart | 2 +- lib/src/importer/utils.dart | 18 +++++ lib/src/js/compile.dart | 4 +- lib/src/js/compile_options.dart | 2 +- lib/src/js/importer.dart | 7 +- test/dart_api/importer_test.dart | 93 ++++++++++++++++++++++++++ test/dart_api/test_importer.dart | 14 +++- 13 files changed, 269 insertions(+), 67 deletions(-) diff --git a/lib/src/async_import_cache.dart b/lib/src/async_import_cache.dart index 019b3414a..33fc26cce 100644 --- a/lib/src/async_import_cache.dart +++ b/lib/src/async_import_cache.dart @@ -122,6 +122,9 @@ final class AsyncImportCache { /// Canonicalizes [url] according to one of this cache's importers. /// + /// The [baseUrl] should be the canonical URL of the stylesheet that contains + /// the load, if it exists. + /// /// Returns the importer that was used to canonicalize [url], the canonical /// URL, and the URL that was passed to the importer (which may be resolved /// relative to [baseUrl] if it's passed). @@ -139,33 +142,30 @@ final class AsyncImportCache { if (isBrowser && (baseImporter == null || baseImporter is NoOpImporter) && _importers.isEmpty) { - throw "Custom importers are required to load stylesheets when compiling in the browser."; + throw "Custom importers are required to load stylesheets when compiling " + "in the browser."; } if (baseImporter != null && url.scheme == '') { - var relativeResult = await putIfAbsentAsync(_relativeCanonicalizeCache, ( - url, - forImport: forImport, - baseImporter: baseImporter, - baseUrl: baseUrl - ), () async { - var resolvedUrl = baseUrl?.resolveUri(url) ?? url; - if (await _canonicalize(baseImporter, resolvedUrl, forImport) - case var canonicalUrl?) { - return (baseImporter, canonicalUrl, originalUrl: resolvedUrl); - } else { - return null; - } - }); + var relativeResult = await putIfAbsentAsync( + _relativeCanonicalizeCache, + ( + url, + forImport: forImport, + baseImporter: baseImporter, + baseUrl: baseUrl + ), + () => _canonicalize(baseImporter, baseUrl?.resolveUri(url) ?? url, + baseUrl, forImport)); if (relativeResult != null) return relativeResult; } return await putIfAbsentAsync( _canonicalizeCache, (url, forImport: forImport), () async { for (var importer in _importers) { - if (await _canonicalize(importer, url, forImport) - case var canonicalUrl?) { - return (importer, canonicalUrl, originalUrl: url); + if (await _canonicalize(importer, url, baseUrl, forImport) + case var result?) { + return result; } } @@ -175,18 +175,36 @@ final class AsyncImportCache { /// Calls [importer.canonicalize] and prints a deprecation warning if it /// returns a relative URL. - Future _canonicalize( - AsyncImporter importer, Uri url, bool forImport) async { - var result = await (forImport - ? inImportRule(() => importer.canonicalize(url)) - : importer.canonicalize(url)); - if (result?.scheme == '') { - _logger.warnForDeprecation(Deprecation.relativeCanonical, """ -Importer $importer canonicalized $url to $result. -Relative canonical URLs are deprecated and will eventually be disallowed. -"""); + /// + /// If [resolveUrl] is `true`, this resolves [url] relative to [baseUrl] + /// before passing it to [importer]. + Future _canonicalize( + AsyncImporter importer, Uri url, Uri? baseUrl, bool forImport, + {bool resolveUrl = false}) async { + var resolved = + resolveUrl && baseUrl != null ? baseUrl.resolveUri(url) : url; + var canonicalize = forImport + ? () => inImportRule(() => importer.canonicalize(resolved)) + : () => importer.canonicalize(resolved); + + var passContainingUrl = baseUrl != null && + (url.scheme == '' || await importer.isNonCanonicalScheme(url.scheme)); + var result = await withContainingUrl( + passContainingUrl ? baseUrl : null, canonicalize); + if (result == null) return null; + + if (result.scheme == '') { + _logger.warnForDeprecation( + Deprecation.relativeCanonical, + "Importer $importer canonicalized $resolved to $result.\n" + "Relative canonical URLs are deprecated and will eventually be " + "disallowed."); + } else if (await importer.isNonCanonicalScheme(result.scheme)) { + throw "Importer $importer canonicalized $resolved to $result, which " + "uses a scheme declared as non-canonical."; } - return result; + + return (importer, result, originalUrl: resolved); } /// Tries to import [url] using one of this cache's importers. diff --git a/lib/src/import_cache.dart b/lib/src/import_cache.dart index b9c48a6f8..bc526d7a3 100644 --- a/lib/src/import_cache.dart +++ b/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: 3e4cae79c03ce2af6626b1822f1468523b401e90 +// Checksum: ff52307a3bc93358ddc46f1e76120894fa3e071f // // ignore_for_file: unused_import @@ -124,6 +124,9 @@ final class ImportCache { /// Canonicalizes [url] according to one of this cache's importers. /// + /// The [baseUrl] should be the canonical URL of the stylesheet that contains + /// the load, if it exists. + /// /// Returns the importer that was used to canonicalize [url], the canonical /// URL, and the URL that was passed to the importer (which may be resolved /// relative to [baseUrl] if it's passed). @@ -139,31 +142,27 @@ final class ImportCache { if (isBrowser && (baseImporter == null || baseImporter is NoOpImporter) && _importers.isEmpty) { - throw "Custom importers are required to load stylesheets when compiling in the browser."; + throw "Custom importers are required to load stylesheets when compiling " + "in the browser."; } if (baseImporter != null && url.scheme == '') { - var relativeResult = _relativeCanonicalizeCache.putIfAbsent(( - url, - forImport: forImport, - baseImporter: baseImporter, - baseUrl: baseUrl - ), () { - var resolvedUrl = baseUrl?.resolveUri(url) ?? url; - if (_canonicalize(baseImporter, resolvedUrl, forImport) - case var canonicalUrl?) { - return (baseImporter, canonicalUrl, originalUrl: resolvedUrl); - } else { - return null; - } - }); + var relativeResult = _relativeCanonicalizeCache.putIfAbsent( + ( + url, + forImport: forImport, + baseImporter: baseImporter, + baseUrl: baseUrl + ), + () => _canonicalize(baseImporter, baseUrl?.resolveUri(url) ?? url, + baseUrl, forImport)); if (relativeResult != null) return relativeResult; } return _canonicalizeCache.putIfAbsent((url, forImport: forImport), () { for (var importer in _importers) { - if (_canonicalize(importer, url, forImport) case var canonicalUrl?) { - return (importer, canonicalUrl, originalUrl: url); + if (_canonicalize(importer, url, baseUrl, forImport) case var result?) { + return result; } } @@ -173,17 +172,36 @@ final class ImportCache { /// Calls [importer.canonicalize] and prints a deprecation warning if it /// returns a relative URL. - Uri? _canonicalize(Importer importer, Uri url, bool forImport) { - var result = (forImport - ? inImportRule(() => importer.canonicalize(url)) - : importer.canonicalize(url)); - if (result?.scheme == '') { - _logger.warnForDeprecation(Deprecation.relativeCanonical, """ -Importer $importer canonicalized $url to $result. -Relative canonical URLs are deprecated and will eventually be disallowed. -"""); + /// + /// If [resolveUrl] is `true`, this resolves [url] relative to [baseUrl] + /// before passing it to [importer]. + CanonicalizeResult? _canonicalize( + Importer importer, Uri url, Uri? baseUrl, bool forImport, + {bool resolveUrl = false}) { + var resolved = + resolveUrl && baseUrl != null ? baseUrl.resolveUri(url) : url; + var canonicalize = forImport + ? () => inImportRule(() => importer.canonicalize(resolved)) + : () => importer.canonicalize(resolved); + + var passContainingUrl = baseUrl != null && + (url.scheme == '' || importer.isNonCanonicalScheme(url.scheme)); + var result = + withContainingUrl(passContainingUrl ? baseUrl : null, canonicalize); + if (result == null) return null; + + if (result.scheme == '') { + _logger.warnForDeprecation( + Deprecation.relativeCanonical, + "Importer $importer canonicalized $resolved to $result.\n" + "Relative canonical URLs are deprecated and will eventually be " + "disallowed."); + } else if (importer.isNonCanonicalScheme(result.scheme)) { + throw "Importer $importer canonicalized $resolved to $result, which " + "uses a scheme declared as non-canonical."; } - return result; + + return (importer, result, originalUrl: resolved); } /// Tries to import [url] using one of this cache's importers. diff --git a/lib/src/importer.dart b/lib/src/importer.dart index 12d626a21..04a08c0f8 100644 --- a/lib/src/importer.dart +++ b/lib/src/importer.dart @@ -40,4 +40,6 @@ abstract class Importer extends AsyncImporter { DateTime modificationTime(Uri url) => DateTime.now(); bool couldCanonicalize(Uri url, Uri canonicalUrl) => true; + + bool isNonCanonicalScheme(String scheme) => false; } diff --git a/lib/src/importer/async.dart b/lib/src/importer/async.dart index 912d05084..31e9fd0ee 100644 --- a/lib/src/importer/async.dart +++ b/lib/src/importer/async.dart @@ -41,6 +41,21 @@ abstract class AsyncImporter { @nonVirtual bool get fromImport => utils.fromImport; + /// The canonical URL of the stylesheet that caused the current [canonicalize] + /// invocation. + /// + /// This is only set when the containing stylesheet has a canonical URL, and + /// when the URL being canonicalized is either relative or has a scheme for + /// which [isNonCanonicalSchemes] returns `true`. This restriction ensures + /// that canonical URLs are always interpreted the same way regardless of + /// their context. + /// + /// Subclasses should only access this from within calls to [canonicalize]. + /// Outside of that context, its value is undefined and subject to change. + @protected + @nonVirtual + Uri? get containingUrl => utils.containingUrl; + /// If [url] is recognized by this importer, returns its canonical format. /// /// Note that canonical URLs *must* be absolute, including a scheme. Returning @@ -137,4 +152,16 @@ abstract class AsyncImporter { /// [url] would actually resolve to [canonicalUrl]. Subclasses are not allowed /// to return false negatives. FutureOr couldCanonicalize(Uri url, Uri canonicalUrl) => true; + + /// Returns whether the given URL scheme (without `:`) should be considered + /// "non-canonical" for this importer. + /// + /// An importer may not return a URL with a non-canonical scheme from + /// [canonicalize]. In exchange, [containingUrl] is available within + /// [canonicalize] for absolute URLs with non-canonical schemes so that the + /// importer can resolve those URLs differently based on where they're loaded. + /// + /// This must always return the same value for the same [scheme]. It is + /// expected to be very efficient. + FutureOr isNonCanonicalScheme(String scheme) => false; } diff --git a/lib/src/importer/js_to_dart/async.dart b/lib/src/importer/js_to_dart/async.dart index 529789d11..adc6de44c 100644 --- a/lib/src/importer/js_to_dart/async.dart +++ b/lib/src/importer/js_to_dart/async.dart @@ -24,7 +24,15 @@ final class JSToDartAsyncImporter extends AsyncImporter { /// The wrapped load function. final Object? Function(JSUrl) _load; - JSToDartAsyncImporter(this._canonicalize, this._load); + /// The set of URL schemes that this importer promises never to return from + /// [canonicalize]. + final Set _nonCanonicalSchemes; + + JSToDartAsyncImporter(this._canonicalize, this._load, + {Iterable? nonCanonicalSchemes}) + : _nonCanonicalSchemes = nonCanonicalSchemes == null + ? const {} + : Set.unmodifiable(nonCanonicalSchemes); FutureOr canonicalize(Uri url) async { var result = wrapJSExceptions(() => _canonicalize( @@ -42,7 +50,7 @@ final class JSToDartAsyncImporter extends AsyncImporter { if (isPromise(result)) result = await promiseToFuture(result as Promise); if (result == null) return null; - result as NodeImporterResult; + result as JSImporterResult; var contents = result.contents; if (!isJsString(contents)) { jsThrow(ArgumentError.value(contents, 'contents', @@ -59,4 +67,7 @@ final class JSToDartAsyncImporter extends AsyncImporter { syntax: parseSyntax(syntax), sourceMapUrl: result.sourceMapUrl.andThen(jsToDartUrl)); } + + bool isNonCanonicalScheme(String scheme) => + _nonCanonicalSchemes.contains(scheme); } diff --git a/lib/src/importer/js_to_dart/file.dart b/lib/src/importer/js_to_dart/file.dart index 3772e87f5..0e9e02e4f 100644 --- a/lib/src/importer/js_to_dart/file.dart +++ b/lib/src/importer/js_to_dart/file.dart @@ -57,4 +57,6 @@ final class JSToDartFileImporter extends Importer { bool couldCanonicalize(Uri url, Uri canonicalUrl) => _filesystemImporter.couldCanonicalize(url, canonicalUrl); + + bool isNonCanonicalSchemes(String scheme) => scheme != 'file'; } diff --git a/lib/src/importer/js_to_dart/sync.dart b/lib/src/importer/js_to_dart/sync.dart index da05420ed..03daf75b3 100644 --- a/lib/src/importer/js_to_dart/sync.dart +++ b/lib/src/importer/js_to_dart/sync.dart @@ -47,7 +47,7 @@ final class JSToDartImporter extends Importer { "functions.")); } - result as NodeImporterResult; + result as JSImporterResult; var contents = result.contents; if (!isJsString(contents)) { jsThrow(ArgumentError.value(contents, 'contents', diff --git a/lib/src/importer/utils.dart b/lib/src/importer/utils.dart index 464a050f8..bebef87f9 100644 --- a/lib/src/importer/utils.dart +++ b/lib/src/importer/utils.dart @@ -17,11 +17,29 @@ import '../io.dart'; /// removed, at which point we can delete this and have one consistent behavior. bool get fromImport => Zone.current[#_inImportRule] as bool? ?? false; +/// The URL of the stylesheet that contains the current load. +Uri? get containingUrl => switch (Zone.current[#_containingUrl]) { + null => throw StateError( + "containingUrl may only be accessed within a call to canonicalize()."), + #_none => null, + Uri url => url, + var value => throw StateError( + "Unexpected Zone.current[#_containingUrl] value $value.") + }; + /// Runs [callback] in a context where [fromImport] returns `true` and /// [resolveImportPath] uses `@import` semantics rather than `@use` semantics. T inImportRule(T callback()) => runZoned(callback, zoneValues: {#_inImportRule: true}); +/// Runs [callback] in a context where [containingUrl] returns [url]. +/// +/// If [when] is `false`, runs [callback] without setting [containingUrl]. +T withContainingUrl(Uri? url, T callback()) => + // Use #_none as a sentinel value so we can distinguish a containing URL + // that's set to null from one that's unset at all. + runZoned(callback, zoneValues: {#_containingUrl: url ?? #_none}); + /// Resolves an imported path using the same logic as the filesystem importer. /// /// This tries to fill in extensions and partial prefixes and check for a diff --git a/lib/src/js/compile.dart b/lib/src/js/compile.dart index af5702308..c71796c75 100644 --- a/lib/src/js/compile.dart +++ b/lib/src/js/compile.dart @@ -184,7 +184,7 @@ OutputStyle _parseOutputStyle(String? style) => switch (style) { AsyncImporter _parseAsyncImporter(Object? importer) { if (importer == null) jsThrow(JsError("Importers may not be null.")); - importer as NodeImporter; + importer as JSImporter; var canonicalize = importer.canonicalize; var load = importer.load; if (importer.findFileUrl case var findFileUrl?) { @@ -208,7 +208,7 @@ AsyncImporter _parseAsyncImporter(Object? importer) { Importer _parseImporter(Object? importer) { if (importer == null) jsThrow(JsError("Importers may not be null.")); - importer as NodeImporter; + importer as JSImporter; var canonicalize = importer.canonicalize; var load = importer.load; if (importer.findFileUrl case var findFileUrl?) { diff --git a/lib/src/js/compile_options.dart b/lib/src/js/compile_options.dart index 603adba4e..1789539d5 100644 --- a/lib/src/js/compile_options.dart +++ b/lib/src/js/compile_options.dart @@ -30,5 +30,5 @@ class CompileOptions { class CompileStringOptions extends CompileOptions { external String? get syntax; external JSUrl? get url; - external NodeImporter? get importer; + external JSImporter? get importer; } diff --git a/lib/src/js/importer.dart b/lib/src/js/importer.dart index 3be952951..30b266a5c 100644 --- a/lib/src/js/importer.dart +++ b/lib/src/js/importer.dart @@ -8,7 +8,7 @@ import 'url.dart'; @JS() @anonymous -class NodeImporter { +class JSImporter { external Object? Function(String, CanonicalizeOptions)? get canonicalize; external Object? Function(JSUrl)? get load; external Object? Function(String, CanonicalizeOptions)? get findFileUrl; @@ -18,13 +18,14 @@ class NodeImporter { @anonymous class CanonicalizeOptions { external bool get fromImport; + external JSUrl get containingUrl; - external factory CanonicalizeOptions({bool fromImport}); + external factory CanonicalizeOptions({bool fromImport, JSUrl containingUrl}); } @JS() @anonymous -class NodeImporterResult { +class JSImporterResult { external String? get contents; external String? get syntax; external JSUrl? get sourceMapUrl; diff --git a/test/dart_api/importer_test.dart b/test/dart_api/importer_test.dart index 113e9a24b..b8ed06a9b 100644 --- a/test/dart_api/importer_test.dart +++ b/test/dart_api/importer_test.dart @@ -112,6 +112,99 @@ void main() { }); }); + group("the containing URL", () { + test("is null for a potentially canonical scheme", () { + late TestImporter importer; + compileString('@import "u:orange";', + importers: [ + importer = TestImporter(expectAsync1((url) { + expect(importer.containingUrl, isNull); + return url; + }), (_) => ImporterResult('', indented: false)) + ], + url: 'x:original.scss'); + }); + + test("throws an error outside canonicalize", () { + late TestImporter importer; + compileString('@import "orange";', importers: [ + importer = + TestImporter((url) => Uri.parse("u:$url"), expectAsync1((url) { + expect(() => importer.containingUrl, throwsStateError); + return ImporterResult('', indented: false); + })) + ]); + }); + + group("for a non-canonical scheme", () { + test("is set to the original URL", () { + late TestImporter importer; + compileString('@import "u:orange";', + importers: [ + importer = TestImporter(expectAsync1((url) { + expect(importer.containingUrl, + equals(Uri.parse('x:original.scss'))); + return url.replace(scheme: 'x'); + }), (_) => ImporterResult('', indented: false), + nonCanonicalSchemes: {'u'}) + ], + url: 'x:original.scss'); + }); + + test("is null if the original URL is null", () { + late TestImporter importer; + compileString('@import "u:orange";', importers: [ + importer = TestImporter(expectAsync1((url) { + expect(importer.containingUrl, isNull); + return url.replace(scheme: 'x'); + }), (_) => ImporterResult('', indented: false), + nonCanonicalSchemes: {'u'}) + ]); + }); + }); + + group("for a schemeless load", () { + test("is set to the original URL", () { + late TestImporter importer; + compileString('@import "orange";', + importers: [ + importer = TestImporter(expectAsync1((url) { + expect(importer.containingUrl, + equals(Uri.parse('x:original.scss'))); + return Uri.parse("u:$url"); + }), (_) => ImporterResult('', indented: false)) + ], + url: 'x:original.scss'); + }); + + test("is null if the original URL is null", () { + late TestImporter importer; + compileString('@import "orange";', importers: [ + importer = TestImporter(expectAsync1((url) { + expect(importer.containingUrl, isNull); + return Uri.parse("u:$url"); + }), (_) => ImporterResult('', indented: false)) + ]); + }); + }); + }); + + test( + "throws an error if the importer returns a canonical URL with a " + "non-canonical scheme", () { + expect( + () => compileString('@import "orange";', importers: [ + TestImporter(expectAsync1((url) => Uri.parse("u:$url")), + (_) => ImporterResult('', indented: false), + nonCanonicalSchemes: {'u'}) + ]), throwsA(predicate((error) { + expect(error, const TypeMatcher()); + expect(error.toString(), + contains("uses a scheme declared as non-canonical")); + return true; + }))); + }); + test("uses an importer's source map URL", () { var result = compileStringToResult('@import "orange";', importers: [ diff --git a/test/dart_api/test_importer.dart b/test/dart_api/test_importer.dart index 87af315df..fda884c93 100644 --- a/test/dart_api/test_importer.dart +++ b/test/dart_api/test_importer.dart @@ -9,10 +9,22 @@ import 'package:sass/sass.dart'; class TestImporter extends Importer { final Uri? Function(Uri url) _canonicalize; final ImporterResult? Function(Uri url) _load; + final Set _nonCanonicalSchemes; - TestImporter(this._canonicalize, this._load); + /// Public access to the containing URL so that [canonicalize] and [load] + /// implementations can access them. + Uri? get publicContainingUrl => containingUrl; + + TestImporter(this._canonicalize, this._load, + {Iterable? nonCanonicalSchemes}) + : _nonCanonicalSchemes = nonCanonicalSchemes == null + ? const {} + : Set.unmodifiable(nonCanonicalSchemes); Uri? canonicalize(Uri url) => _canonicalize(url); ImporterResult? load(Uri url) => _load(url); + + bool isNonCanonicalScheme(String scheme) => + _nonCanonicalSchemes.contains(scheme); }