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.

Sign up for GitHub

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

Jump to bottom

Cache separate canonical URLs for @use and @import #908

Merged
merged 4 commits into from Jan 2, 2020
Merged

Cache separate canonical URLs for @use and @import #908

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
588cd31
Cache separate canonical URLs for @use and @import
jathak Dec 26, 2019
295a8f9
Split StylesheetNode.upstream into two maps
jathak Dec 27, 2019
c640388
Code review
jathak Dec 28, 2019
dcd129a
Revert math changes to allow 1.24.1 release
jathak Jan 2, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
3 changes: 3 additions & 0 deletions CHANGELOG.md
View file
Open in desktop
Expand Up @@ -8,6 +8,9 @@

* Add the variables `$pi` and `$e` to the built-in "sass:math" module.

* 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
32 changes: 22 additions & 10 deletions lib/src/async_import_cache.dart
View file
Open in desktop
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,11 @@ 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 {
var result = await (forImport
? inImportRule(() => importer.canonicalize(url))
: importer.canonicalize(url));
if (result?.scheme == '') {
_logger.warn("""
Importer $importer canonicalized $url to $result.
Expand All @@ -153,8 +163,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 +227,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
20 changes: 14 additions & 6 deletions lib/src/executable/watch.dart
View file
Open in desktop
Expand Up @@ -288,8 +288,9 @@ class _Watcher {
var changed = <StylesheetNode>[];
for (var node in _graph.nodes.values) {
var importChanged = false;
for (var url in node.upstream.keys) {
if (_name(p.url.basename(url.path)) != name) continue;
var forImport = false;
void recanonicalize(Uri url, StylesheetNode node) {
if (_name(p.url.basename(url.path)) != name) return;
_graph.clearCanonicalize(url);

// If the import produces a different canonicalized URL than it did
Expand All @@ -298,15 +299,22 @@ class _Watcher {
Uri newCanonicalUrl;
try {
newCanonicalUrl = _graph.importCache
.canonicalize(url, node.importer, node.canonicalUrl)
.canonicalize(url,
baseImporter: node.importer,
baseUrl: node.canonicalUrl,
forImport: forImport)
?.item2;
} catch (_) {
// If the call to canonicalize failed, do nothing. We'll surface the
// error more nicely when we try to recompile the file.
// If the call to canonicalize failed, do nothing. We'll surface
// the error more nicely when we try to recompile the file.
}
importChanged = newCanonicalUrl != node.upstream[url]?.canonicalUrl;
importChanged = newCanonicalUrl != node?.canonicalUrl;
}
}

node.upstream.forEach(recanonicalize);
forImport = true;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Rather than using this as implicit state, I'd just pass it to recanonicalize(). It does mean you'll have to expand those forEach() calls, but I think the clarity of flow is worth it.

node.upstreamImports.forEach(recanonicalize);
if (importChanged) changed.add(node);
}

Expand Down
30 changes: 19 additions & 11 deletions lib/src/import_cache.dart
View file
Open in desktop
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: 3ca2f221c3c1503c688be49d4f7501bd9be8031a
//
// 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,10 @@ 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) {
var result = (forImport
? inImportRule(() => importer.canonicalize(url))
: importer.canonicalize(url));
if (result?.scheme == '') {
_logger.warn("""
Importer $importer canonicalized $url to $result.
Expand All @@ -158,8 +164,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 +227,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
Open in desktop
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 for 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;