From 5740eb541faaf97c70a451924809b9f212f2920e Mon Sep 17 00:00:00 2001 From: Natalie Weizenbaum Date: Tue, 3 Dec 2024 14:55:11 -0800 Subject: [PATCH] Make several changes to improve the reliability of `--watch` mode (#2444) 1. The import cache now tracks the most recent time it actually loaded a stylesheet, so that `--watch` mode can invalidate cached data if it's older than the mtime on disk. This should help catch some cases where the watcher information doesn't match the filesystem. 2. Rather than eagerly recompiling as soon as it knows it needs to, the stylesheet graph now only starts recompiling once it's finished processing a batch of events. This ensures that any cache invalidation is finished before the recompilation happens. 3. The stylesheet graph and import cache now eagerly invalidate all canonicalize results that could be changed by an added or removed file, rather than only those that are implicated by the in-memory ASTs. This avoids issues when the in-memory AST is stale. Closes #2440 --- CHANGELOG.md | 7 +- lib/src/async_import_cache.dart | 36 +++++++-- lib/src/executable/compile_stylesheet.dart | 4 + lib/src/executable/watch.dart | 90 +++++++++++----------- lib/src/import_cache.dart | 38 ++++++--- lib/src/stylesheet_graph.dart | 30 +++++++- pkg/sass-parser/CHANGELOG.md | 2 +- pkg/sass-parser/package.json | 2 +- pkg/sass_api/CHANGELOG.md | 2 +- pkg/sass_api/pubspec.yaml | 2 +- pubspec.yaml | 2 +- 11 files changed, 145 insertions(+), 70 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 7a3cc8fcb..c6730d729 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,4 +1,9 @@ -## 1.82.0-dev +## 1.82.0 + +### Command-Line Interface + +* Improve `--watch` mode reliability when making multiple changes at once, such + as checking out a different Git branch. * Parse the `calc-size()` function as a calculation now that it's supported in some browsers. diff --git a/lib/src/async_import_cache.dart b/lib/src/async_import_cache.dart index 0bc3b4da7..b07735664 100644 --- a/lib/src/async_import_cache.dart +++ b/lib/src/async_import_cache.dart @@ -69,6 +69,10 @@ final class AsyncImportCache { /// The import results for each canonicalized import URL. final _resultsCache = {}; + /// A map from canonical URLs to the most recent time at which those URLs were + /// loaded from their importers. + final _loadTimes = {}; + /// Creates an import cache that resolves imports using [importers]. /// /// Imports are resolved by trying, in order: @@ -282,9 +286,11 @@ final class AsyncImportCache { Future importCanonical(AsyncImporter importer, Uri canonicalUrl, {Uri? originalUrl}) async { return await putIfAbsentAsync(_importCache, canonicalUrl, () async { + var loadTime = DateTime.now(); var result = await importer.load(canonicalUrl); if (result == null) return null; + _loadTimes[canonicalUrl] = loadTime; _resultsCache[canonicalUrl] = result; return Stylesheet.parse(result.contents, result.syntax, // For backwards-compatibility, relative canonical URLs are resolved @@ -320,17 +326,31 @@ final class AsyncImportCache { Uri sourceMapUrl(Uri canonicalUrl) => _resultsCache[canonicalUrl]?.sourceMapUrl ?? canonicalUrl; - /// Clears the cached canonical version of the given non-canonical [url]. - /// - /// Has no effect if the canonical version of [url] has not been cached. + /// Returns the most recent time the stylesheet at [canonicalUrl] was loaded + /// from its importer, or `null` if it has never been loaded. + @internal + DateTime? loadTime(Uri canonicalUrl) => _loadTimes[canonicalUrl]; + + /// Clears all cached canonicalizations that could potentially produce + /// [canonicalUrl]. /// /// @nodoc @internal - void clearCanonicalize(Uri url) { - _canonicalizeCache.remove((url, forImport: false)); - _canonicalizeCache.remove((url, forImport: true)); - _perImporterCanonicalizeCache.removeWhere( - (key, _) => key.$2 == url || _nonCanonicalRelativeUrls[key] == url); + Future clearCanonicalize(Uri canonicalUrl) async { + for (var key in [..._canonicalizeCache.keys]) { + for (var importer in _importers) { + if (await importer.couldCanonicalize(key.$1, canonicalUrl)) { + _canonicalizeCache.remove(key); + break; + } + } + } + + for (var key in [..._perImporterCanonicalizeCache.keys]) { + if (await key.$1.couldCanonicalize(key.$2, canonicalUrl)) { + _perImporterCanonicalizeCache.remove(key); + } + } } /// Clears the cached parse tree for the stylesheet with the given diff --git a/lib/src/executable/compile_stylesheet.dart b/lib/src/executable/compile_stylesheet.dart index c2f26d260..50bdfaac3 100644 --- a/lib/src/executable/compile_stylesheet.dart +++ b/lib/src/executable/compile_stylesheet.dart @@ -124,6 +124,10 @@ Future _compileStylesheetWithoutErrorHandling(ExecutableOptions options, fatalDeprecations: options.fatalDeprecations, futureDeprecations: options.futureDeprecations); } else { + // Double-check that all modified files (according to mtime) are actually + // reloaded in the graph so we don't end up with stale ASTs. + graph.reloadAllModified(); + result = source == null ? compileString(await readStdin(), syntax: syntax, diff --git a/lib/src/executable/watch.dart b/lib/src/executable/watch.dart index 9e1db78e9..000bf3201 100644 --- a/lib/src/executable/watch.dart +++ b/lib/src/executable/watch.dart @@ -62,6 +62,10 @@ final class _Watcher { /// The graph of stylesheets being compiled. final StylesheetGraph _graph; + /// A map from source paths to destinations that need to be recompiled once + /// the current batch of events has been processed. + final Map _toRecompile = {}; + _Watcher(this._options, this._graph); /// Deletes the file at [path] and prints a message about it. @@ -82,32 +86,39 @@ final class _Watcher { /// /// Returns a future that will only complete if an unexpected error occurs. Future watch(MultiDirWatcher watcher) async { - await for (var event in _debounceEvents(watcher.events)) { - var extension = p.extension(event.path); - if (extension != '.sass' && extension != '.scss' && extension != '.css') { - continue; + await for (var batch in _debounceEvents(watcher.events)) { + for (var event in batch) { + var extension = p.extension(event.path); + if (extension != '.sass' && + extension != '.scss' && + extension != '.css') { + continue; + } + + switch (event.type) { + case ChangeType.MODIFY: + _handleModify(event.path); + + case ChangeType.ADD: + _handleAdd(event.path); + + case ChangeType.REMOVE: + _handleRemove(event.path); + } } - switch (event.type) { - case ChangeType.MODIFY: - var success = await _handleModify(event.path); - if (!success && _options.stopOnError) return; - - case ChangeType.ADD: - var success = await _handleAdd(event.path); - if (!success && _options.stopOnError) return; - - case ChangeType.REMOVE: - var success = await _handleRemove(event.path); - if (!success && _options.stopOnError) return; - } + var toRecompile = {..._toRecompile}; + _toRecompile.clear(); + var success = await compileStylesheets(_options, _graph, toRecompile, + ifModified: true); + if (!success && _options.stopOnError) return; } } /// Handles a modify event for the stylesheet at [path]. /// /// Returns whether all necessary recompilations succeeded. - Future _handleModify(String path) async { + void _handleModify(String path) { var url = _canonicalize(path); // It's important to access the node ahead-of-time because it's possible @@ -115,29 +126,27 @@ final class _Watcher { // from the graph. if (_graph.nodes[url] case var node?) { _graph.reload(url); - return await _recompileDownstream([node]); + _recompileDownstream([node]); } else { - return _handleAdd(path); + _handleAdd(path); } } /// Handles an add event for the stylesheet at [url]. /// /// Returns whether all necessary recompilations succeeded. - Future _handleAdd(String path) async { + void _handleAdd(String path) { var destination = _destinationFor(path); - var success = destination == null || - await compileStylesheets(_options, _graph, {path: destination}, - ifModified: true); + if (destination != null) _toRecompile[path] = destination; var downstream = _graph.addCanonical( FilesystemImporter.cwd, _canonicalize(path), p.toUri(path)); - return await _recompileDownstream(downstream) && success; + _recompileDownstream(downstream); } /// Handles a remove event for the stylesheet at [url]. /// /// Returns whether all necessary recompilations succeeded. - Future _handleRemove(String path) async { + void _handleRemove(String path) async { var url = _canonicalize(path); if (_graph.nodes.containsKey(url)) { @@ -145,7 +154,7 @@ final class _Watcher { } var downstream = _graph.remove(FilesystemImporter.cwd, url); - return await _recompileDownstream(downstream); + _recompileDownstream(downstream); } /// Returns the canonical URL for the stylesheet path [path]. @@ -154,9 +163,10 @@ final class _Watcher { /// Combine [WatchEvent]s that happen in quick succession. /// /// Otherwise, if a file is erased and then rewritten, we can end up reading - /// the intermediate erased version. - Stream _debounceEvents(Stream events) { - return events.debounceBuffer(Duration(milliseconds: 25)).expand((buffer) { + /// the intermediate erased version. This returns a stream of batches of + /// events that all happened in succession. + Stream> _debounceEvents(Stream events) { + return events.debounceBuffer(Duration(milliseconds: 25)).map((buffer) { var typeForPath = p.PathMap(); for (var event in buffer) { var oldType = typeForPath[event.path]; @@ -175,32 +185,20 @@ final class _Watcher { }); } - /// Recompiles [nodes] and everything that transitively imports them, if - /// necessary. - /// - /// Returns whether all recompilations succeeded. - Future _recompileDownstream(Iterable nodes) async { + /// Marks [nodes] and everything that transitively imports them for + /// recompilation, if necessary. + void _recompileDownstream(Iterable nodes) { var seen = {}; - var allSucceeded = true; while (nodes.isNotEmpty) { nodes = [ for (var node in nodes) if (seen.add(node)) node ]; - var sourcesToDestinations = _sourceEntrypointsToDestinations(nodes); - if (sourcesToDestinations.isNotEmpty) { - var success = await compileStylesheets( - _options, _graph, sourcesToDestinations, - ifModified: true); - if (!success && _options.stopOnError) return false; - - allSucceeded = allSucceeded && success; - } + _toRecompile.addAll(_sourceEntrypointsToDestinations(nodes)); nodes = [for (var node in nodes) ...node.downstream]; } - return allSucceeded; } /// Returns a sourcesToDestinations mapping for nodes that are entrypoints. diff --git a/lib/src/import_cache.dart b/lib/src/import_cache.dart index e9c627fb1..b6d38d15f 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: 4d09da97db4e59518d193f58963897d36ef4db00 +// Checksum: 65a7c538299527be3240f0625a7c1cd4f8cd6824 // // ignore_for_file: unused_import @@ -70,6 +70,10 @@ final class ImportCache { /// The import results for each canonicalized import URL. final _resultsCache = {}; + /// A map from canonical URLs to the most recent time at which those URLs were + /// loaded from their importers. + final _loadTimes = {}; + /// Creates an import cache that resolves imports using [importers]. /// /// Imports are resolved by trying, in order: @@ -276,9 +280,11 @@ final class ImportCache { Stylesheet? importCanonical(Importer importer, Uri canonicalUrl, {Uri? originalUrl}) { return _importCache.putIfAbsent(canonicalUrl, () { + var loadTime = DateTime.now(); var result = importer.load(canonicalUrl); if (result == null) return null; + _loadTimes[canonicalUrl] = loadTime; _resultsCache[canonicalUrl] = result; return Stylesheet.parse(result.contents, result.syntax, // For backwards-compatibility, relative canonical URLs are resolved @@ -314,17 +320,31 @@ final class ImportCache { Uri sourceMapUrl(Uri canonicalUrl) => _resultsCache[canonicalUrl]?.sourceMapUrl ?? canonicalUrl; - /// Clears the cached canonical version of the given non-canonical [url]. - /// - /// Has no effect if the canonical version of [url] has not been cached. + /// Returns the most recent time the stylesheet at [canonicalUrl] was loaded + /// from its importer, or `null` if it has never been loaded. + @internal + DateTime? loadTime(Uri canonicalUrl) => _loadTimes[canonicalUrl]; + + /// Clears all cached canonicalizations that could potentially produce + /// [canonicalUrl]. /// /// @nodoc @internal - void clearCanonicalize(Uri url) { - _canonicalizeCache.remove((url, forImport: false)); - _canonicalizeCache.remove((url, forImport: true)); - _perImporterCanonicalizeCache.removeWhere( - (key, _) => key.$2 == url || _nonCanonicalRelativeUrls[key] == url); + void clearCanonicalize(Uri canonicalUrl) { + for (var key in [..._canonicalizeCache.keys]) { + for (var importer in _importers) { + if (importer.couldCanonicalize(key.$1, canonicalUrl)) { + _canonicalizeCache.remove(key); + break; + } + } + } + + for (var key in [..._perImporterCanonicalizeCache.keys]) { + if (key.$1.couldCanonicalize(key.$2, canonicalUrl)) { + _perImporterCanonicalizeCache.remove(key); + } + } } /// Clears the cached parse tree for the stylesheet with the given diff --git a/lib/src/stylesheet_graph.dart b/lib/src/stylesheet_graph.dart index 245d8b146..5e03cf458 100644 --- a/lib/src/stylesheet_graph.dart +++ b/lib/src/stylesheet_graph.dart @@ -8,6 +8,7 @@ import 'package:path/path.dart' as p; import 'ast/sass.dart'; import 'import_cache.dart'; import 'importer.dart'; +import 'io.dart'; import 'util/map.dart'; import 'util/nullable.dart'; import 'visitor/find_dependencies.dart'; @@ -169,6 +170,33 @@ class StylesheetGraph { return true; } + /// Re-parses all stylesheets in the graph that have been modified on disk + /// since their last known in-memory modification. + /// + /// This guards against situations where a recompilation is triggered before + /// the graph is manually informed of all changes, such as when `--poll` runs + /// slowly or native file system notifications aren't comprehensive. + void reloadAllModified() { + // Copy to a list because [reload] can modify [_nodes]. + for (var node in [..._nodes.values]) { + var modified = false; + try { + var loadTime = importCache.loadTime(node.canonicalUrl); + modified = loadTime != null && + node.importer.modificationTime(node.canonicalUrl).isAfter(loadTime); + } on FileSystemException catch (_) { + // If the file no longer exists, treat that as a modification. + modified = true; + } + + if (modified) { + if (!reload(node.canonicalUrl)) { + remove(node.importer, node.canonicalUrl); + } + } + } + } + /// Removes the stylesheet at [canonicalUrl] (loaded by [importer]) from the /// stylesheet graph. /// @@ -204,6 +232,7 @@ class StylesheetGraph { /// Returns all nodes whose imports were changed. Set _recanonicalizeImports( Importer importer, Uri canonicalUrl) { + importCache.clearCanonicalize(canonicalUrl); var changed = {}; for (var node in nodes.values) { var newUpstream = _recanonicalizeImportsForNode( @@ -242,7 +271,6 @@ class StylesheetGraph { var newMap = {}; for (var (url, upstream) in map.pairs) { if (!importer.couldCanonicalize(url, canonicalUrl)) continue; - importCache.clearCanonicalize(url); // If the import produces a different canonicalized URL than it did // before, it changed and the stylesheet needs to be recompiled. diff --git a/pkg/sass-parser/CHANGELOG.md b/pkg/sass-parser/CHANGELOG.md index a8d43624c..621869053 100644 --- a/pkg/sass-parser/CHANGELOG.md +++ b/pkg/sass-parser/CHANGELOG.md @@ -1,4 +1,4 @@ -## 0.4.7-dev +## 0.4.7 * No user-visible changes. diff --git a/pkg/sass-parser/package.json b/pkg/sass-parser/package.json index fa9bebc8f..bd9bc108c 100644 --- a/pkg/sass-parser/package.json +++ b/pkg/sass-parser/package.json @@ -1,6 +1,6 @@ { "name": "sass-parser", - "version": "0.4.7-dev", + "version": "0.4.7", "description": "A PostCSS-compatible wrapper of the official Sass parser", "repository": "sass/sass", "author": "Google Inc.", diff --git a/pkg/sass_api/CHANGELOG.md b/pkg/sass_api/CHANGELOG.md index a2d14f2e6..b6af1a883 100644 --- a/pkg/sass_api/CHANGELOG.md +++ b/pkg/sass_api/CHANGELOG.md @@ -1,4 +1,4 @@ -## 14.4.0-dev +## 14.4.0 * No user-visible changes. diff --git a/pkg/sass_api/pubspec.yaml b/pkg/sass_api/pubspec.yaml index 068bab853..a3f48aa05 100644 --- a/pkg/sass_api/pubspec.yaml +++ b/pkg/sass_api/pubspec.yaml @@ -2,7 +2,7 @@ name: sass_api # Note: Every time we add a new Sass AST node, we need to bump the *major* # version because it's a breaking change for anyone who's implementing the # visitor interface(s). -version: 14.4.0-dev +version: 14.4.0 description: Additional APIs for Dart Sass. homepage: https://github.com/sass/dart-sass diff --git a/pubspec.yaml b/pubspec.yaml index a6873d61c..589fb93cc 100644 --- a/pubspec.yaml +++ b/pubspec.yaml @@ -1,5 +1,5 @@ name: sass -version: 1.82.0-dev +version: 1.82.0 description: A Sass implementation in Dart. homepage: https://github.com/sass/dart-sass