From 3b92c128b6da68e0779635244780a89b2685a30b Mon Sep 17 00:00:00 2001 From: Jennifer Thakar Date: Fri, 27 Dec 2019 16:58:13 -0800 Subject: [PATCH] Code review --- lib/src/executable/watch.dart | 14 +++++++------ lib/src/stylesheet_graph.dart | 28 ++++++++++++++------------ lib/src/visitor/find_dependencies.dart | 9 ++++----- 3 files changed, 27 insertions(+), 24 deletions(-) diff --git a/lib/src/executable/watch.dart b/lib/src/executable/watch.dart index f596c0d5e..13bf164e1 100644 --- a/lib/src/executable/watch.dart +++ b/lib/src/executable/watch.dart @@ -288,8 +288,7 @@ class _Watcher { var changed = []; for (var node in _graph.nodes.values) { var importChanged = false; - var forImport = false; - void recanonicalize(Uri url, StylesheetNode node) { + void recanonicalize(Uri url, StylesheetNode upstream, bool forImport) { if (_name(p.url.basename(url.path)) != name) return; _graph.clearCanonicalize(url); @@ -308,13 +307,16 @@ class _Watcher { // 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?.canonicalUrl; + importChanged = newCanonicalUrl != upstream?.canonicalUrl; } } - node.upstream.forEach(recanonicalize); - forImport = true; - node.upstreamImports.forEach(recanonicalize); + for (var entry in node.upstream.entries) { + recanonicalize(entry.key, entry.value, false); + } + for (var entry in node.upstreamImports.entries) { + recanonicalize(entry.key, entry.value, true); + } if (importChanged) changed.add(node); } diff --git a/lib/src/stylesheet_graph.dart b/lib/src/stylesheet_graph.dart index f4b9a6190..556f8f8e2 100644 --- a/lib/src/stylesheet_graph.dart +++ b/lib/src/stylesheet_graph.dart @@ -40,10 +40,8 @@ class StylesheetGraph { DateTime transitiveModificationTime(StylesheetNode node) { return _transitiveModificationTimes.putIfAbsent(node.canonicalUrl, () { var latest = node.importer.modificationTime(node.canonicalUrl); - for (var upstream in { - ...node.upstream.values, - ...node.upstreamImports.values - }) { + for (var upstream + in node.upstream.values.followedBy(node.upstreamImports.values)) { // If an import is missing, always recompile so we show the user the // error. var upstreamTime = upstream == null @@ -94,18 +92,20 @@ class StylesheetGraph { _upstreamNodes(stylesheet, importer, canonicalUrl))); } - /// Returns a map from non-canonicalized imported URLs in [stylesheet], which - /// appears within [baseUrl] imported by [baseImporter]. + /// Returns two maps from non-canonicalized imported URLs in [stylesheet] to + /// nodes, which appears within [baseUrl] imported by [baseImporter]. + /// + /// The first map contains stylesheets depended on via `@use` and `@forward` + /// while the second map contains those depended on via `@import`. Tuple2, Map> _upstreamNodes( Stylesheet stylesheet, Importer baseImporter, Uri baseUrl) { var active = {baseUrl}; var tuple = findDependencies(stylesheet); - var importUrls = tuple.item2.map((import) => Uri.parse(import.url)); return Tuple2({ for (var url in tuple.item1) url: _nodeFor(url, baseImporter, baseUrl, active) }, { - for (var url in importUrls) + for (var url in tuple.item2) url: _nodeFor(url, baseImporter, baseUrl, active, forImport: true) }); } @@ -244,7 +244,7 @@ class StylesheetNode { /// A map from non-canonicalized `@use` and `@forward` URLs in [stylesheet] to /// the stylesheets those rules refer to. /// - /// This may have `null` values, which indicate failed imports. + /// This may have `null` values, which indicate failed loads. Map get upstream => UnmodifiableMapView(_upstream); Map _upstream; @@ -264,14 +264,16 @@ class StylesheetNode { Tuple2, Map> allUpstream) : _upstream = allUpstream.item1, _upstreamImports = allUpstream.item2 { - for (var node in {...upstream.values, ...upstreamImports.values}) { + for (var node in upstream.values.followedBy(upstreamImports.values)) { if (node != null) node._downstream.add(this); } } - /// Sets [newUpstream] as the new value of [upstream] and [newUpstreamImports] - /// as the new value of [upstreamImports], and adjusts upstream nodes' - /// [downstream] fields accordingly. + /// Updates [upstream] and [upstreamImports] from [allUpstream] and adjusts + /// upstream nodes' [downstream] fields accordingly. + /// + /// The first item in [allUpstream] replaces [upstream] while the second item + /// replaces [upstreamImports]. void _replaceUpstream( Tuple2, Map> allUpstream) { var oldUpstream = {...upstream.values, ...upstreamImports.values} diff --git a/lib/src/visitor/find_dependencies.dart b/lib/src/visitor/find_dependencies.dart index 9e077c38b..d2963ec50 100644 --- a/lib/src/visitor/find_dependencies.dart +++ b/lib/src/visitor/find_dependencies.dart @@ -11,17 +11,16 @@ import 'recursive_statement.dart'; /// /// The first is a list of URLs from all `@use` and `@forward` rules in /// [stylesheet]. The second is a list of all imports in [stylesheet]. -Tuple2, List> findDependencies( - Stylesheet stylesheet) => +Tuple2, List> findDependencies(Stylesheet stylesheet) => _FindDependenciesVisitor().run(stylesheet); /// A visitor that traverses a stylesheet and records, all `@import`, `@use`, /// and `@forward` rules it contains. class _FindDependenciesVisitor extends RecursiveStatementVisitor { final _usesAndForwards = []; - final _imports = []; + final _imports = []; - Tuple2, List> run(Stylesheet stylesheet) { + Tuple2, List> run(Stylesheet stylesheet) { visitStylesheet(stylesheet); return Tuple2(_usesAndForwards, _imports); } @@ -45,7 +44,7 @@ class _FindDependenciesVisitor extends RecursiveStatementVisitor { void visitImportRule(ImportRule node) { for (var import in node.imports) { - if (import is DynamicImport) _imports.add(import); + if (import is DynamicImport) _imports.add(Uri.parse(import.url)); } } }