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

jathak · 2019-12-26T22:21:28Z

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.

lib/src/async_import_cache.dart

lib/src/importer/utils.dart

nex3 · 2019-12-27T21:28:10Z

lib/src/stylesheet_graph.dart

@@ -92,14 +93,17 @@ class StylesheetGraph {

  /// Returns a map from non-canonicalized imported URLs in [stylesheet], which


Document the new map value.

It looks like this doc comment still needs to be updated.

nex3 · 2019-12-27T21:31:03Z

lib/src/stylesheet_graph.dart

@@ -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 =>


It's plausible that we'll want to expose this class publicly relatively soon, and I really don't want to have to break this API once we remove imports. WDYT about making upstream refer only to @use and @import, and have a separate upstreamImports field that only contains imports?

Yeah, that makes sense. I went back and forth on whether to key on a tuple or just have two maps while I was writing this. Would it make sense to do the same in ImportCache for consistency?

Since ImportCache's map isn't part of its public API, I think it's fine to have it stay as tuple-keyed.

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.

nex3 · 2019-12-28T00:12:26Z

lib/src/executable/watch.dart

        }
      }
+
+      node.upstream.forEach(recanonicalize);
+      forImport = true;


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.

nex3 · 2019-12-28T00:14:21Z

lib/src/stylesheet_graph.dart

-        for (var upstream in node.upstream.values) {
+        for (var upstream in {
+          ...node.upstream.values,
+          ...node.upstreamImports.values


I'd use Iterable.followedBy() here to avoid copying everything into a set eagerly. There's a possibility we'll visit a file twice, but that seems unlikely and it won't affect the result.

Similar below.

nex3 · 2019-12-28T00:15:44Z

lib/src/stylesheet_graph.dart

@@ -92,14 +93,17 @@ class StylesheetGraph {

  /// Returns a map from non-canonicalized imported URLs in [stylesheet], which


It looks like this doc comment still needs to be updated.

nex3 · 2019-12-28T00:20:10Z

lib/src/visitor/find_dependencies.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<Uri>, List<DynamicImport>> findDependencies(


Why aren't imports represented as Uris as well?

nex3 · 2019-12-28T00:20:52Z

lib/src/stylesheet_graph.dart

-  /// A map from non-canonicalized import URLs in [stylesheet] to the
-  /// stylesheets those imports refer to.
+  /// 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.


Suggested change

/// This may have `null` values, which indicate failed imports.

/// This may have `null` values, which indicate failed loads.

jathak · 2020-01-02T18:00:25Z

@nex3: Can you restart the AppVeyor build? I don't think I have permission

nex3 · 2020-01-02T20:40:03Z

Done.

nex3 · 2020-01-02T20:50:29Z

lib/src/executable/watch.dart

@@ -288,8 +288,8 @@ 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;
+      void recanonicalize(Uri url, StylesheetNode upstream, bool forImport) {


Nit:

Suggested change

void recanonicalize(Uri url, StylesheetNode upstream, bool forImport) {

void recanonicalize(Uri url, StylesheetNode upstream, {@required bool forImport}) {

This just helps make it clearer at the callsite what the boolean means.

jathak requested a review from nex3 December 26, 2019 22:21

jathak force-pushed the cache-fix branch from ef75dbf to 2970460 Compare December 26, 2019 22:52

nex3 reviewed Dec 27, 2019

View reviewed changes

jathak requested a review from nex3 December 27, 2019 23:31

jathak added 2 commits December 27, 2019 15:33

Cache separate canonical URLs for @use and @import

588cd31

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.

Split StylesheetNode.upstream into two maps

295a8f9

jathak force-pushed the cache-fix branch from 7087076 to 295a8f9 Compare December 27, 2019 23:33

nex3 reviewed Dec 28, 2019

View reviewed changes

jathak requested a review from nex3 January 2, 2020 20:41

nex3 approved these changes Jan 2, 2020

View reviewed changes

Code review

c640388

jathak force-pushed the cache-fix branch from 3b92c12 to c640388 Compare January 2, 2020 20:53

Revert math changes to allow 1.24.1 release

dcd129a

jathak merged commit 79d9a73 into master Jan 2, 2020

jathak deleted the cache-fix branch January 2, 2020 23:25

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

jathak commented Dec 26, 2019

nex3 Dec 27, 2019

nex3 Dec 28, 2019

nex3 Dec 27, 2019

jathak Dec 27, 2019

nex3 Dec 27, 2019

nex3 Dec 28, 2019

nex3 Dec 28, 2019

nex3 Dec 28, 2019

nex3 Dec 28, 2019

nex3 Dec 28, 2019

jathak commented Jan 2, 2020

nex3 commented Jan 2, 2020

nex3 Jan 2, 2020

		@@ -92,14 +93,17 @@ class StylesheetGraph {

		/// Returns a map from non-canonicalized imported URLs in [stylesheet], which

	/// This may have `null` values, which indicate failed imports.
	/// This may have `null` values, which indicate failed loads.

	void recanonicalize(Uri url, StylesheetNode upstream, bool forImport) {
	void recanonicalize(Uri url, StylesheetNode upstream, {@required bool forImport}) {

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

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

Conversation

jathak commented Dec 26, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jathak commented Jan 2, 2020

nex3 commented Jan 2, 2020

Choose a reason for hiding this comment