Merge pull request #962 from sass/watch-bug

Fix a --watch bug

Author: nex3

CHANGELOG.md

@@ -1,3 +1,11 @@
+## 1.26.1
+
+### Command Line Interface
+
+* Fix a longstanding bug where `--watch` mode could enter into a state where
+  recompilation would not occur after a syntax error was introduced into a
+  dependency and then fixed.
+
 ## 1.26.0
 
 * **Potentially breaking bug fix:** `@use` rules whose URLs' basenames begin

lib/src/async_import_cache.dart

@@ -112,7 +112,8 @@ class AsyncImportCache {
   /// canonicalize [url] (resolved relative to [baseUrl] if it's passed).
   ///
   /// If any importers understand [url], returns that importer as well as the
-  /// canonicalized URL. Otherwise, returns `null`.
+  /// canonicalized URL and the original URL resolved relative to [baseUrl] if
+  /// applicable. Otherwise, returns `null`.
   Future<Tuple3<AsyncImporter, Uri, Uri>> canonicalize(Uri url,
       {AsyncImporter baseImporter, Uri baseUrl, bool forImport = false}) async {
     if (baseImporter != null) {

lib/src/executable/watch.dart

@@ -5,7 +5,6 @@
 import 'dart:async';
 import 'dart:collection';
 
-import 'package:meta/meta.dart';
 import 'package:path/path.dart' as p;
 import 'package:stack_trace/stack_trace.dart';
 import 'package:stream_transform/stream_transform.dart';
@@ -44,7 +43,8 @@ Future<void> watch(ExecutableOptions options, StylesheetGraph graph) async {
   for (var source in options.sourcesToDestinations.keys) {
     var destination = options.sourcesToDestinations[source];
     graph.addCanonical(FilesystemImporter('.'), p.toUri(p.canonicalize(source)),
-        p.toUri(source));
+        p.toUri(source),
+        recanonicalize: false);
     var success = await watcher.compile(source, destination, ifModified: true);
     if (!success && options.stopOnError) {
       dirWatcher.events.listen(null).cancel();
@@ -165,32 +165,26 @@ class _Watcher {
   ///
   /// Returns whether all necessary recompilations succeeded.
   Future<bool> _handleAdd(String path) async {
-    var success = await _retryPotentialImports(path);
-    if (!success && _options.stopOnError) return false;
-
     var destination = _destinationFor(path);
-    if (destination == null) return true;
 
-    _graph.addCanonical(
+    var success = destination == null || await compile(path, destination);
+    var downstream = _graph.addCanonical(
         FilesystemImporter('.'), _canonicalize(path), p.toUri(path));
-
-    return await compile(path, destination);
+    return await _recompileDownstream(downstream) && success;
   }
 
   /// Handles a remove event for the stylesheet at [url].
   ///
   /// Returns whether all necessary recompilations succeeded.
   Future<bool> _handleRemove(String path) async {
     var url = _canonicalize(path);
-    var success = await _retryPotentialImports(path);
-    if (!success && _options.stopOnError) return false;
-    if (!_graph.nodes.containsKey(url)) return true;
 
-    var destination = _destinationFor(path);
-    if (destination != null) _delete(destination);
+    if (_graph.nodes.containsKey(url)) {
+      var destination = _destinationFor(path);
+      if (destination != null) _delete(destination);
+    }
 
-    var downstream = _graph.nodes[url].downstream;
-    _graph.remove(url);
+    var downstream = _graph.remove(FilesystemImporter('.'), url);
     return await _recompileDownstream(downstream);
   }
 
@@ -278,56 +272,4 @@ class _Watcher {
 
     return null;
   }
-
-  /// Re-runs all imports in [_graph] that might refer to [path], and recompiles
-  /// the files that contain those imports if they end up importing new
-  /// stylesheets.
-  ///
-  /// Returns whether all recompilations succeeded.
-  Future<bool> _retryPotentialImports(String path) async {
-    var name = _name(p.basename(path));
-    var changed = <StylesheetNode>[];
-    for (var node in _graph.nodes.values) {
-      var importChanged = false;
-      void recanonicalize(Uri url, StylesheetNode upstream,
-          {@required bool forImport}) {
-        if (_name(p.url.basename(url.path)) != name) return;
-        _graph.clearCanonicalize(url);
-
-        // If the import produces a different canonicalized URL than it did
-        // before, it changed and the stylesheet needs to be recompiled.
-        if (!importChanged) {
-          Uri newCanonicalUrl;
-          try {
-            newCanonicalUrl = _graph.importCache
-                .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.
-          }
-          importChanged = newCanonicalUrl != upstream?.canonicalUrl;
-        }
-      }
-
-      for (var entry in node.upstream.entries) {
-        recanonicalize(entry.key, entry.value, forImport: false);
-      }
-      for (var entry in node.upstreamImports.entries) {
-        recanonicalize(entry.key, entry.value, forImport: true);
-      }
-      if (importChanged) changed.add(node);
-    }
-
-    return await _recompileDownstream(changed);
-  }
-
-  /// Removes an extension from [extension], and a leading underscore if it has one.
-  String _name(String basename) {
-    basename = p.withoutExtension(basename);
-    return basename.startsWith("_") ? basename.substring(1) : basename;
-  }
 }

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: 3ca2f221c3c1503c688be49d4f7501bd9be8031a
+// Checksum: c1dd0f2a41b03bd1227a2358688bf44499b9d90c
 //
 // ignore_for_file: unused_import
 
@@ -116,7 +116,8 @@ class ImportCache {
   /// canonicalize [url] (resolved relative to [baseUrl] if it's passed).
   ///
   /// If any importers understand [url], returns that importer as well as the
-  /// canonicalized URL. Otherwise, returns `null`.
+  /// canonicalized URL and the original URL resolved relative to [baseUrl] if
+  /// applicable. Otherwise, returns `null`.
   Tuple3<Importer, Uri, Uri> canonicalize(Uri url,
       {Importer baseImporter, Uri baseUrl, bool forImport = false}) {
     if (baseImporter != null) {

lib/src/importer.dart

@@ -36,4 +36,6 @@ abstract class Importer extends AsyncImporter {
   ImporterResult load(Uri url);
 
   DateTime modificationTime(Uri url) => DateTime.now();
+
+  bool couldCanonicalize(Uri url, Uri canonicalUrl) => true;
 }

lib/src/importer/async.dart

@@ -107,4 +107,13 @@ abstract class AsyncImporter {
   /// If this throws an exception, the exception is ignored and the current time
   /// is used as the modification time.
   FutureOr<DateTime> modificationTime(Uri url) => DateTime.now();
+
+  /// Without accessing the filesystem, returns whether or not passing [url] to
+  /// [canonicalize] could possibly return [canonicalUrl].
+  ///
+  /// This is expected to be very efficient, and subclasses are allowed to
+  /// return false positives if it would be inefficient to determine whether
+  /// [url] would actually resolve to [canonicalUrl]. Subclasses are not allowed
+  /// to return false negatives.
+  FutureOr<bool> couldCanonicalize(Uri url, Uri canonicalUrl) => true;
 }

lib/src/importer/filesystem.dart

@@ -39,5 +39,19 @@ class FilesystemImporter extends Importer {
 
   DateTime modificationTime(Uri url) => io.modificationTime(p.fromUri(url));
 
+  bool couldCanonicalize(Uri url, Uri canonicalUrl) {
+    if (url.scheme != 'file' && url.scheme != '') return false;
+    if (canonicalUrl.scheme != 'file') return false;
+
+    var basename = p.url.basename(url.path);
+    var canonicalBasename = p.url.basename(canonicalUrl.path);
+    if (!basename.startsWith("_") && canonicalBasename.startsWith("_")) {
+      canonicalBasename = canonicalBasename.substring(1);
+    }
+
+    return basename == canonicalBasename ||
+        basename == p.url.withoutExtension(canonicalBasename);
+  }
+
   String toString() => _loadPath;
 }

lib/src/importer/no_op.dart

@@ -12,6 +12,7 @@ import 'result.dart';
 class NoOpImporter extends Importer {
   Uri canonicalize(Uri url) => null;
   ImporterResult load(Uri url) => null;
+  bool couldCanonicalize(Uri url, Uri canonicalUrl) => false;
 
   String toString() => "(unknown)";
 }

lib/src/importer/package.dart

@@ -47,5 +47,9 @@ class PackageImporter extends Importer {
   DateTime modificationTime(Uri url) =>
       _filesystemImporter.modificationTime(url);
 
+  bool couldCanonicalize(Uri url, Uri canonicalUrl) =>
+      (url.scheme == 'file' || url.scheme == 'package' || url.scheme == '') &&
+      _filesystemImporter.couldCanonicalize(Uri(path: url.path), canonicalUrl);
+
   String toString() => "package:...";
 }