From 5fccc8c4db7313d48d53ae205af80945faff17a5 Mon Sep 17 00:00:00 2001 From: Natalie Weizenbaum Date: Thu, 21 Mar 2024 16:44:10 -0700 Subject: [PATCH] Better handle filesystem importers when load paths aren't necessary See #2199 See sass/sass#3815 --- CHANGELOG.md | 23 ++++++++++ bin/sass.dart | 3 +- lib/src/deprecation.dart | 5 +++ lib/src/importer/filesystem.dart | 74 ++++++++++++++++++++++++++++---- 4 files changed, 95 insertions(+), 10 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 3d430a121..c5ae69c7c 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -3,8 +3,31 @@ * Add support for nesting in plain CSS files. This is not processed by Sass at all; it's emitted exactly as-is in the CSS. +* In certain circumstances, the current working directory was unintentionally + being made available as a load path. This is now deprecated. Anyone relying on + this should explicitly pass in `.` as a load path or `FilesystemImporter('.')` + as the current importer. + * Add linux-riscv64 and windows-arm64 releases. +### Command-Line Interface + +* Fix a bug where absolute `file:` URLs weren't loaded for files compiled via + the command line unless an unrelated load path was also passed. + +* Fix a bug where `--update` would always update files that were specified via + absolute path unless an unrelated load path was also passed. + +### Dart API + +* Add `FilesystemImporter.noLoadPath`, which is a `FilesystemImporter` that can + load absolute `file:` URLs and resolve URLs relative to the base file but + doesn't load relative URLs from a load path. + +* `FilesystemImporter.cwd` is now deprecated. Either use + `FilesystemImporter.noLoadPath` if you weren't intending to rely on the load + path, or `FilesystemImporter('.')` if you were. + ## 1.72.0 * Support adjacent `/`s without whitespace in between when parsing plain CSS diff --git a/bin/sass.dart b/bin/sass.dart index 67b1782ed..d24439eb9 100644 --- a/bin/sass.dart +++ b/bin/sass.dart @@ -13,6 +13,7 @@ import 'package:sass/src/executable/options.dart'; import 'package:sass/src/executable/repl.dart'; import 'package:sass/src/executable/watch.dart'; import 'package:sass/src/import_cache.dart'; +import 'package:sass/src/importer/filesystem.dart'; import 'package:sass/src/io.dart'; import 'package:sass/src/logger/deprecation_handling.dart'; import 'package:sass/src/stylesheet_graph.dart'; @@ -45,7 +46,7 @@ Future main(List args) async { } var graph = StylesheetGraph(ImportCache( - importers: options.pkgImporters, + importers: [...options.pkgImporters, FilesystemImporter.noLoadPath], loadPaths: options.loadPaths, // This logger is only used for handling fatal/future deprecations // during parsing, and is re-used across parses, so we don't want to diff --git a/lib/src/deprecation.dart b/lib/src/deprecation.dart index 2724d0afe..423536235 100644 --- a/lib/src/deprecation.dart +++ b/lib/src/deprecation.dart @@ -69,6 +69,11 @@ enum Deprecation { deprecatedIn: '1.62.3', description: 'Passing null as alpha in the ${isJS ? 'JS' : 'Dart'} API.'), + fsImporterCwd('fs-importer-cwd', + deprecatedIn: '1.73.0', + description: + 'Using the current working directory as an implicit load ' 'path.'), + @Deprecated('This deprecation name was never actually used.') calcInterp('calc-interp', deprecatedIn: null), diff --git a/lib/src/importer/filesystem.dart b/lib/src/importer/filesystem.dart index 47b0ae288..8754666a1 100644 --- a/lib/src/importer/filesystem.dart +++ b/lib/src/importer/filesystem.dart @@ -5,30 +5,86 @@ import 'package:meta/meta.dart'; import 'package:path/path.dart' as p; +import '../deprecation.dart'; +import '../evaluation_context.dart'; import '../importer.dart'; import '../io.dart' as io; import '../syntax.dart'; import '../util/nullable.dart'; import 'utils.dart'; -/// An importer that loads files from a load path on the filesystem. +/// An importer that loads files from a load path on the filesystem, either +/// relative to the path passed to [FilesystemImporter.new] or absolute `file:` +/// URLs. +/// +/// Use [FilesystemImporter.noLoadPath] to _only_ load absolute `file:` URLs and +/// URLs relative to the current file. /// /// {@category Importer} @sealed class FilesystemImporter extends Importer { /// The path relative to which this importer looks for files. - final String _loadPath; + /// + /// If this is `null`, this importer will _only_ load absolute `file:` URLs + /// and URLs relative to the current file. + final String? _loadPath; + + /// Whether loading from files from this importer's [_loadPath] is deprecated. + final bool _loadPathDeprecated; /// Creates an importer that loads files relative to [loadPath]. - FilesystemImporter(String loadPath) : _loadPath = p.absolute(loadPath); + FilesystemImporter(String loadPath) + : _loadPath = p.absolute(loadPath), + _loadPathDeprecated = false; + + FilesystemImporter._deprecated(String loadPath) + : _loadPath = p.absolute(loadPath), + _loadPathDeprecated = true; + + /// Creates an importer that _only_ loads absolute `file:` URLs and URLs + /// relative to the current file. + FilesystemImporter._noLoadPath() + : _loadPath = null, + _loadPathDeprecated = false; - /// Creates an importer relative to the current working directory. - static final cwd = FilesystemImporter('.'); + /// A [FilesystemImporter] that loads files relative to the current working + /// directory. + /// + /// Historically, this was the best default for supporting `file:` URL loads + /// when the load path didn't matter. However, adding the current working + /// directory to the load path wasn't always desirable, so it's no longer + /// recommneded. Instead, either use [FilesystemImporter.noLoadPath] if the + /// load path doesn't matter, or `FilesystemImporter('.')` to explicitly + /// preserve the existing behavior. + @Deprecated( + "Use FilesystemImporter.noLoadPath or FilesystemImporter('.') instead.") + static final cwd = FilesystemImporter._deprecated('.'); + + /// Creates an importer that _only_ loads absolute `file:` URLsand URLs + /// relative to the current file. + static final noLoadPath = FilesystemImporter._noLoadPath(); Uri? canonicalize(Uri url) { - if (url.scheme != 'file' && url.scheme != '') return null; - return resolveImportPath(p.join(_loadPath, p.fromUri(url))) - .andThen((resolved) => p.toUri(io.canonicalize(resolved))); + String? resolved; + if (url.scheme == 'file') { + resolved = resolveImportPath(p.fromUri(url)); + } else if (url.scheme != '') { + return null; + } else if (_loadPath case var loadPath?) { + if (_loadPathDeprecated) { + warnForDeprecation( + "Using the current working directory as an implicit load path is " + "deprecated. Either add it as an explicit load path or importer, or " + "load this stylesheet from a different URL.", + Deprecation.fsImporterCwd); + } + + resolved = resolveImportPath(p.join(loadPath, p.fromUri(url))); + } else { + return null; + } + + return resolved.andThen((resolved) => p.toUri(io.canonicalize(resolved))); } ImporterResult? load(Uri url) { @@ -53,5 +109,5 @@ class FilesystemImporter extends Importer { basename == p.url.withoutExtension(canonicalBasename); } - String toString() => _loadPath; + String toString() => _loadPath ?? ''; }