Skip to content

Commit

Permalink
Cache separate canonical URLs for @use and @import
Browse files Browse the repository at this point in the history 
Fixes #899.

The cache for canonical URLs is now keyed on *both* the rule URL and
whether that URL was canonicalized for an `@import` rule.
  • Loading branch information
@jathak
jathak committed Dec 26, 2019
1 parent 24f84e2 commit ef75dbf
Show file tree
Hide file tree
Showing 10 changed files with 150 additions and 86 deletions.
3 changes: 3 additions & 0 deletions CHANGELOG.md
View file
Expand Up @@ -6,6 +6,9 @@
* `hypot()`: given *n* numbers, outputs the length of the *n*-dimensional
vector that has components equal to each of the inputs.

* Fix a bug where the wrong file could be loaded when the same URL is used by
both a `@use` rule and an `@import` rule.

## 1.24.0

* Add an optional `with` clause to the `@forward` rule. This works like the
Expand Down
35 changes: 25 additions & 10 deletions lib/src/async_import_cache.dart
View file
Expand Up @@ -11,6 +11,7 @@ import 'package:tuple/tuple.dart';
import 'ast/sass.dart';
import 'importer.dart';
import 'importer/result.dart';
import 'importer/utils.dart';
import 'io.dart';
import 'logger.dart';
import 'sync_package_resolver.dart';
Expand All @@ -26,11 +27,15 @@ class AsyncImportCache {

/// The canonicalized URLs for each non-canonical URL.
///
/// The second item in each key's tuple is true when this canonicalization is
/// for an `@import` rule. Otherwise, it's for a `@use` or `@forward` rule.
///
/// This map's values are the same as the return value of [canonicalize].
///
/// This cache isn't used for relative imports, because they're
/// context-dependent.
final Map<Uri, Tuple3<AsyncImporter, Uri, Uri>> _canonicalizeCache;
final Map<Tuple2<Uri, bool>, Tuple3<AsyncImporter, Uri, Uri>>
_canonicalizeCache;

/// The parsed stylesheets for each canonicalized import URL.
final Map<Uri, Stylesheet> _importCache;
Expand Down Expand Up @@ -109,18 +114,20 @@ class AsyncImportCache {
/// If any importers understand [url], returns that importer as well as the
/// canonicalized URL. Otherwise, returns `null`.
Future<Tuple3<AsyncImporter, Uri, Uri>> canonicalize(Uri url,
[AsyncImporter baseImporter, Uri baseUrl]) async {
{AsyncImporter baseImporter, Uri baseUrl, bool forImport = false}) async {
if (baseImporter != null) {
var resolvedUrl = baseUrl != null ? baseUrl.resolveUri(url) : url;
var canonicalUrl = await _canonicalize(baseImporter, resolvedUrl);
var canonicalUrl =
await _canonicalize(baseImporter, resolvedUrl, forImport);
if (canonicalUrl != null) {
return Tuple3(baseImporter, canonicalUrl, resolvedUrl);
}
}

return await putIfAbsentAsync(_canonicalizeCache, url, () async {
return await putIfAbsentAsync(_canonicalizeCache, Tuple2(url, forImport),
() async {
for (var importer in _importers) {
var canonicalUrl = await _canonicalize(importer, url);
var canonicalUrl = await _canonicalize(importer, url, forImport);
if (canonicalUrl != null) {
return Tuple3(importer, canonicalUrl, url);
}
Expand All @@ -132,8 +139,14 @@ class AsyncImportCache {

/// Calls [importer.canonicalize] and prints a deprecation warning if it
/// returns a relative URL.
Future<Uri> _canonicalize(AsyncImporter importer, Uri url) async {
var result = await importer.canonicalize(url);
Future<Uri> _canonicalize(
AsyncImporter importer, Uri url, bool forImport) async {
Uri result;
if (forImport) {
result = await inImportRule(() => importer.canonicalize(url));
} else {
result = await importer.canonicalize(url);
}
if (result?.scheme == '') {
_logger.warn("""
Importer $importer canonicalized $url to $result.
Expand All @@ -153,8 +166,9 @@ Relative canonical URLs are deprecated and will eventually be disallowed.
///
/// Caches the result of the import and uses cached results if possible.
Future<Tuple2<AsyncImporter, Stylesheet>> import(Uri url,
[AsyncImporter baseImporter, Uri baseUrl]) async {
var tuple = await canonicalize(url, baseImporter, baseUrl);
{AsyncImporter baseImporter, Uri baseUrl, bool forImport = false}) async {
var tuple = await canonicalize(url,
baseImporter: baseImporter, baseUrl: baseUrl, forImport: forImport);
if (tuple == null) return null;
var stylesheet =
await importCanonical(tuple.item1, tuple.item2, tuple.item3);
Expand Down Expand Up @@ -216,7 +230,8 @@ Relative canonical URLs are deprecated and will eventually be disallowed.
///
/// Has no effect if the canonical version of [url] has not been cached.
void clearCanonicalize(Uri url) {
_canonicalizeCache.remove(url);
_canonicalizeCache.remove(Tuple2(url, false));
_canonicalizeCache.remove(Tuple2(url, true));
}

/// Clears the cached parse tree for the stylesheet with the given
Expand Down
8 changes: 6 additions & 2 deletions lib/src/executable/watch.dart
View file
Expand Up @@ -288,7 +288,8 @@ class _Watcher {
var changed = <StylesheetNode>[];
for (var node in _graph.nodes.values) {
var importChanged = false;
for (var url in node.upstream.keys) {
for (var tuple in node.upstream.keys) {
var url = tuple.item1;
if (_name(p.url.basename(url.path)) != name) continue;
_graph.clearCanonicalize(url);

Expand All @@ -298,7 +299,10 @@ class _Watcher {
Uri newCanonicalUrl;
try {
newCanonicalUrl = _graph.importCache
.canonicalize(url, node.importer, node.canonicalUrl)
.canonicalize(url,
baseImporter: node.importer,
baseUrl: node.canonicalUrl,
forImport: tuple.item2)
?.item2;
} catch (_) {
// If the call to canonicalize failed, do nothing. We'll surface the
Expand Down
33 changes: 22 additions & 11 deletions lib/src/import_cache.dart
View file
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: 8555cca43b8d54d392e81f33935fd379d1eb3c72
// Checksum: e3bcbb88321ab00d5c7341fabd792633be874f07
//
// ignore_for_file: unused_import

Expand All @@ -16,6 +16,7 @@ import 'package:tuple/tuple.dart';
import 'ast/sass.dart';
import 'importer.dart';
import 'importer/result.dart';
import 'importer/utils.dart';
import 'io.dart';
import 'logger.dart';
import 'sync_package_resolver.dart';
Expand All @@ -31,11 +32,14 @@ class ImportCache {

/// The canonicalized URLs for each non-canonical URL.
///
/// The second item in each key's tuple is true when this canonicalization is
/// for an `@import` rule. Otherwise, it's for a `@use` or `@forward` rule.
///
/// This map's values are the same as the return value of [canonicalize].
///
/// This cache isn't used for relative imports, because they're
/// context-dependent.
final Map<Uri, Tuple3<Importer, Uri, Uri>> _canonicalizeCache;
final Map<Tuple2<Uri, bool>, Tuple3<Importer, Uri, Uri>> _canonicalizeCache;

/// The parsed stylesheets for each canonicalized import URL.
final Map<Uri, Stylesheet> _importCache;
Expand Down Expand Up @@ -114,18 +118,18 @@ class ImportCache {
/// If any importers understand [url], returns that importer as well as the
/// canonicalized URL. Otherwise, returns `null`.
Tuple3<Importer, Uri, Uri> canonicalize(Uri url,
[Importer baseImporter, Uri baseUrl]) {
{Importer baseImporter, Uri baseUrl, bool forImport = false}) {
if (baseImporter != null) {
var resolvedUrl = baseUrl != null ? baseUrl.resolveUri(url) : url;
var canonicalUrl = _canonicalize(baseImporter, resolvedUrl);
var canonicalUrl = _canonicalize(baseImporter, resolvedUrl, forImport);
if (canonicalUrl != null) {
return Tuple3(baseImporter, canonicalUrl, resolvedUrl);
}
}

return _canonicalizeCache.putIfAbsent(url, () {
return _canonicalizeCache.putIfAbsent(Tuple2(url, forImport), () {
for (var importer in _importers) {
var canonicalUrl = _canonicalize(importer, url);
var canonicalUrl = _canonicalize(importer, url, forImport);
if (canonicalUrl != null) {
return Tuple3(importer, canonicalUrl, url);
}
Expand All @@ -137,8 +141,13 @@ class ImportCache {

/// Calls [importer.canonicalize] and prints a deprecation warning if it
/// returns a relative URL.
Uri _canonicalize(Importer importer, Uri url) {
var result = importer.canonicalize(url);
Uri _canonicalize(Importer importer, Uri url, bool forImport) {
Uri result;
if (forImport) {
result = inImportRule(() => importer.canonicalize(url));
} else {
result = importer.canonicalize(url);
}
if (result?.scheme == '') {
_logger.warn("""
Importer $importer canonicalized $url to $result.
Expand All @@ -158,8 +167,9 @@ Relative canonical URLs are deprecated and will eventually be disallowed.
///
/// Caches the result of the import and uses cached results if possible.
Tuple2<Importer, Stylesheet> import(Uri url,
[Importer baseImporter, Uri baseUrl]) {
var tuple = canonicalize(url, baseImporter, baseUrl);
{Importer baseImporter, Uri baseUrl, bool forImport = false}) {
var tuple = canonicalize(url,
baseImporter: baseImporter, baseUrl: baseUrl, forImport: forImport);
if (tuple == null) return null;
var stylesheet = importCanonical(tuple.item1, tuple.item2, tuple.item3);
return Tuple2(tuple.item1, stylesheet);
Expand Down Expand Up @@ -220,7 +230,8 @@ Relative canonical URLs are deprecated and will eventually be disallowed.
///
/// Has no effect if the canonical version of [url] has not been cached.
void clearCanonicalize(Uri url) {
_canonicalizeCache.remove(url);
_canonicalizeCache.remove(Tuple2(url, false));
_canonicalizeCache.remove(Tuple2(url, true));
}

/// Clears the cached parse tree for the stylesheet with the given
Expand Down
45 changes: 22 additions & 23 deletions lib/src/importer/utils.dart
View file
Expand Up @@ -6,43 +6,42 @@ import 'package:path/path.dart' as p;

import '../io.dart';

/// Whether the Sass interpreter is currently evaluating a `@use` rule.
/// Whether the Sass compiler is currently evaluating an `@import` rule.
///
/// The `@use` rule has slightly different path-resolution behavior than
/// `@import`: `@use` prioritizes a `.css` file with a given name at the same
/// level as `.sass` and `.scss`, while `@import` prefers `.sass` and `.scss`
/// over `.css`. It's admittedly hacky to set this globally, but `@import` will
/// eventually be removed, at which point we can delete this and have one
/// consistent behavior.
bool _inUseRule = false;
/// When evaluating `@import` rules, URLs should canonicalize to an import-only
/// file if one exists for the URL being canonicalized. Otherwise,
/// canonicalization should be identical for `@import` and `@use` rules. It's
/// admittedly hacky to set this globally, but `@import` will eventually be
/// removed, at which point we can delete this and have one consistent behavior.
bool _inImportRule = false;

/// Runs [callback] in a context where [resolveImportPath] uses `@use` semantics
/// rather than `@import` semantics.
T inUseRule<T>(T callback()) {
var wasInUseRule = _inUseRule;
_inUseRule = true;
/// Runs [callback] in a context where [resolveImportPath] uses `@import`
/// semantics rather than `@use` semantics.
T inImportRule<T>(T callback()) {
var wasInImportRule = _inImportRule;
_inImportRule = true;
try {
return callback();
} finally {
_inUseRule = wasInUseRule;
_inImportRule = wasInImportRule;
}
}

/// Like [inUseRule], but asynchronous.
Future<T> inUseRuleAsync<T>(Future<T> callback()) async {
var wasInUseRule = _inUseRule;
_inUseRule = true;
/// Like [inImportRule], but asynchronous.
Future<T> inImportRuleAsync<T>(Future<T> callback()) async {
var wasInImportRule = _inImportRule;
_inImportRule = true;
try {
return await callback();
} finally {
_inUseRule = wasInUseRule;
_inImportRule = wasInImportRule;
}
}

/// Resolves an imported path using the same logic as the filesystem importer.
///
/// This tries to fill in extensions and partial prefixes and check if a directory default. If no file can be
/// found, it returns `null`.
/// This tries to fill in extensions and partial prefixes and check if a
/// directory default. If no file can be found, it returns `null`.
String resolveImportPath(String path) {
var extension = p.extension(path);
if (extension == '.sass' || extension == '.scss' || extension == '.css') {
Expand Down Expand Up @@ -96,7 +95,7 @@ String _exactlyOne(List<String> paths) {
paths.map((path) => " " + p.prettyUri(p.toUri(path))).join("\n");
}

/// If [_inUseRule] is `false`, invokes callback and returns the result.
/// If [_inImportRule] is `true`, invokes callback and returns the result.
///
/// Otherwise, returns `null`.
T _ifInImport<T>(T callback()) => _inUseRule ? null : callback();
T _ifInImport<T>(T callback()) => _inImportRule ? callback() : null;
34 changes: 20 additions & 14 deletions lib/src/stylesheet_graph.dart
View file
Expand Up @@ -5,11 +5,12 @@
import 'dart:collection';

import 'package:collection/collection.dart';
import 'package:tuple/tuple.dart';

import 'ast/sass.dart';
import 'import_cache.dart';
import 'importer.dart';
import 'visitor/find_imports.dart';
import 'visitor/find_dependencies.dart';

/// A graph of the import relationships between stylesheets, available via
/// [nodes].
Expand Down Expand Up @@ -64,8 +65,8 @@ class StylesheetGraph {
///
/// Returns `null` if the import cache can't find a stylesheet at [url].
StylesheetNode _add(Uri url, [Importer baseImporter, Uri baseUrl]) {
var tuple = _ignoreErrors(
() => importCache.canonicalize(url, baseImporter, baseUrl));
var tuple = _ignoreErrors(() => importCache.canonicalize(url,
baseImporter: baseImporter, baseUrl: baseUrl));
if (tuple == null) return null;
return addCanonical(tuple.item1, tuple.item2, tuple.item3);
}
Expand All @@ -92,14 +93,17 @@ class StylesheetGraph {

/// Returns a map from non-canonicalized imported URLs in [stylesheet], which
/// appears within [baseUrl] imported by [baseImporter].
Map<Uri, StylesheetNode> _upstreamNodes(
Map<Tuple2<Uri, bool>, StylesheetNode> _upstreamNodes(
Stylesheet stylesheet, Importer baseImporter, Uri baseUrl) {
var active = {baseUrl};
var upstreamUrls =
findImports(stylesheet).map((import) => Uri.parse(import.url));
var tuple = findDependencies(stylesheet);
var importUrls = tuple.item2.map((import) => Uri.parse(import.url));
return {
for (var url in upstreamUrls)
url: _nodeFor(url, baseImporter, baseUrl, active)
for (var url in tuple.item1)
Tuple2(url, false): _nodeFor(url, baseImporter, baseUrl, active),
for (var url in importUrls)
Tuple2(url, true):
_nodeFor(url, baseImporter, baseUrl, active, forImport: true)
};
}

Expand Down Expand Up @@ -160,9 +164,10 @@ class StylesheetGraph {
/// The [active] set should contain the canonical URLs that are currently
/// being imported. It's used to detect circular imports.
StylesheetNode _nodeFor(
Uri url, Importer baseImporter, Uri baseUrl, Set<Uri> active) {
var tuple = _ignoreErrors(
() => importCache.canonicalize(url, baseImporter, baseUrl));
Uri url, Importer baseImporter, Uri baseUrl, Set<Uri> active,
{bool forImport = false}) {
var tuple = _ignoreErrors(() => importCache.canonicalize(url,
baseImporter: baseImporter, baseUrl: baseUrl, forImport: forImport));

// If an import fails, let the evaluator surface that error rather than
// surfacing it here.
Expand Down Expand Up @@ -237,8 +242,9 @@ class StylesheetNode {
/// stylesheets those imports refer to.
///
/// This may have `null` values, which indicate failed imports.
Map<Uri, StylesheetNode> get upstream => UnmodifiableMapView(_upstream);
Map<Uri, StylesheetNode> _upstream;
Map<Tuple2<Uri, bool>, StylesheetNode> get upstream =>
UnmodifiableMapView(_upstream);
Map<Tuple2<Uri, bool>, StylesheetNode> _upstream;

/// The stylesheets that import [stylesheet].
Set<StylesheetNode> get downstream => UnmodifiableSetView(_downstream);
Expand All @@ -253,7 +259,7 @@ class StylesheetNode {

/// Sets [newUpstream] as the new value of [upstream] and adjusts upstream
/// nodes' [downstream] fields accordingly.
void _replaceUpstream(Map<Uri, StylesheetNode> newUpstream) {
void _replaceUpstream(Map<Tuple2<Uri, bool>, StylesheetNode> newUpstream) {
var oldUpstream = Set.of(upstream.values)..remove(null);
var newUpstreamSet = Set.of(newUpstream.values)..remove(null);

Expand Down

0 comments on commit ef75dbf

Please sign in to comment.