Skip to content

Commit a1bad96

Browse files
authored
Merge pull request #178 from sass/more-fixes
Fix a few more bugs
2 parents 73cbc9e + d68a177 commit a1bad96

13 files changed

+212
-92
lines changed

CHANGELOG.md

+10
Original file line numberDiff line numberDiff line change
@@ -1,3 +1,13 @@
1+
## 1.3.5
2+
3+
### Module Migrator
4+
5+
* Fix a bug where `@use` rules could be duplicated if the same file is depended
6+
on via both an indirect `@import` and an existing `@use` rule.
7+
8+
* Fix a bug where imports of orphan import-only files that only forward other
9+
import-only files would not be removed.
10+
111
## 1.3.4
212

313
### Module Migrator

lib/src/migrators/module.dart

+42-27
Original file line numberDiff line numberDiff line change
@@ -138,8 +138,8 @@ class _ModuleMigrationVisitor extends MigrationVisitor {
138138

139139
/// Set of canonical URLs that have a `@use` rule in the current stylesheet.
140140
///
141-
/// This includes both `@use` rules migrated from `@import` rules and
142-
/// additional `@use` rules in the sets below.
141+
/// This includes rules migrated from `@import` rules, additional rules in the
142+
/// sets below, and existing `@use` rules in the file prior to migration.
143143
Set<Uri> _usedUrls;
144144

145145
/// Set of additional `@use` rules for built-in modules.
@@ -402,9 +402,17 @@ class _ModuleMigrationVisitor extends MigrationVisitor {
402402
var oldBeforeFirstImport = _beforeFirstImport;
403403
var oldAfterLastImport = _afterLastImport;
404404
var oldUseAllowed = _useAllowed;
405-
_namespaces = _determineNamespaces(node.span.sourceUrl);
406-
_forwardedUrls = {};
405+
_namespaces = {
406+
for (var rule in node.uses)
407+
importCache
408+
.canonicalize(rule.url,
409+
baseImporter: importer, baseUrl: node.span.sourceUrl)
410+
?.item2 ??
411+
rule.url: rule.namespace
412+
};
413+
_determineNamespaces(node.span.sourceUrl, _namespaces);
407414
_usedUrls = {};
415+
_forwardedUrls = {};
408416
_builtInUseRules = {};
409417
_additionalLoadPathUseRules = {};
410418
_additionalRelativeUseRules = {};
@@ -451,30 +459,27 @@ class _ModuleMigrationVisitor extends MigrationVisitor {
451459
}
452460
// Add a line break after the extra rules if there's not already a blank
453461
// line after the insertion point.
454-
var whitespace = extendThroughWhitespace(insertionPoint.pointSpan());
462+
var whitespace = insertionPoint.pointSpan().extendThroughWhitespace();
455463
if (!whitespace.text.contains('\n\n') && whitespace.end != node.span.end) {
456464
extras = '$extras\n'; //extras.substring(0, extras.length - 1);
457465
}
458466
addPatch(Patch.insert(insertionPoint, extras));
459467
}
460468

461-
/// Determines namespaces for all `@use` rules that the stylesheet at [url]
462-
/// will contain after migration.
463-
Map<Uri, String> _determineNamespaces(Uri url) {
464-
var namespaces = <Uri, String>{};
469+
/// Adds namespaces for the `@use` rules added to [url] by the migration.
470+
///
471+
/// [namespaces] should contain any existing namespaces prior to migration
472+
/// (i.e. from existing `@use` rules).
473+
void _determineNamespaces(Uri url, Map<Uri, String> namespaces) {
465474
var sourcesByNamespace = <String, Set<ReferenceSource>>{};
466475
for (var reference in references.sources.keys) {
467476
if (reference.span.sourceUrl != url) continue;
468477
var source = references.sources[reference];
478+
if (source is UseSource) continue;
479+
if (namespaces.containsKey(source.url)) continue;
469480
var namespace = source.preferredNamespace;
470481
if (namespace == null) continue;
471-
472-
// Existing `@use` rules should always keep their namespaces.
473-
if (source is UseSource) {
474-
namespaces[source.url] = namespace;
475-
} else {
476-
sourcesByNamespace.putIfAbsent(namespace, () => {}).add(source);
477-
}
482+
sourcesByNamespace.putIfAbsent(namespace, () => {}).add(source);
478483
}
479484
// First assign namespaces to module URLs without conflicts.
480485
var conflictingNamespaces = <String, Set<ReferenceSource>>{};
@@ -490,7 +495,6 @@ class _ModuleMigrationVisitor extends MigrationVisitor {
490495
conflictingNamespaces.forEach((namespace, sources) {
491496
_resolveNamespaceConflict(namespace, sources, namespaces, url);
492497
});
493-
return namespaces;
494498
}
495499

496500
/// Resolves a conflict between a set of sources with the same default
@@ -765,6 +769,7 @@ class _ModuleMigrationVisitor extends MigrationVisitor {
765769
baseImporter: importer, forImport: true);
766770
var canonicalImport = tuple?.item2;
767771
if (references.orphanImportOnlyFiles.containsKey(canonicalImport)) {
772+
ruleUrl = null;
768773
var url = references.orphanImportOnlyFiles[canonicalImport]?.url;
769774
if (url != null) {
770775
var canonicalRedirect = importCache
@@ -786,8 +791,13 @@ class _ModuleMigrationVisitor extends MigrationVisitor {
786791
}
787792

788793
rulesText = migratedRules.join('$_semicolonIfNotIndented\n$indent');
789-
790-
addPatch(Patch(node.span, rulesText));
794+
if (rulesText.isEmpty) {
795+
var span =
796+
node.span.extendIfMatches(RegExp(' *$_semicolonIfNotIndented\n?'));
797+
addPatch(patchDelete(span));
798+
} else {
799+
addPatch(Patch(node.span, rulesText));
800+
}
791801
if (_useAllowed) {
792802
_beforeFirstImport ??= node.span.start;
793803
_afterLastImport = afterImport(node,
@@ -1074,7 +1084,7 @@ class _ModuleMigrationVisitor extends MigrationVisitor {
10741084
_renameReference(nameSpan(node), declaration);
10751085
var namespace = _namespaceForDeclaration(declaration);
10761086
if (namespace != null) {
1077-
addPatch(Patch(subspan(nameSpan(node), end: 0), '$namespace.'));
1087+
addPatch(Patch.insert(nameSpan(node).start, '$namespace.'));
10781088
}
10791089
}
10801090

@@ -1086,21 +1096,27 @@ class _ModuleMigrationVisitor extends MigrationVisitor {
10861096
super.visitMixinRule(node);
10871097
}
10881098

1089-
/// If [node] is for a built-in module, adds its URL to [_usedUrls] so we
1090-
/// don't add a duplicate one, but ignore other `@use` rules, as we'll assume
1091-
/// they've already been migrated.
1099+
/// Don't visit `@use` rules.
10921100
///
10931101
/// The migrator will use the information from [references] to migrate
10941102
/// references to members of these dependencies.
10951103
void visitUseRule(UseRule node) {
1096-
if (node.url.scheme == 'sass') _usedUrls.add(node.url);
1104+
_usedUrls.add(importCache
1105+
.canonicalize(node.url, baseImporter: importer, baseUrl: currentUrl)
1106+
?.item2 ??
1107+
node.url);
10971108
}
10981109

10991110
/// Similar to `@use` rules, don't visit `@forward` rules.
11001111
///
11011112
/// The migrator will use the information from [references] to migrate
11021113
/// references to members of these dependencies.
1103-
void visitForwardRule(ForwardRule node) {}
1114+
void visitForwardRule(ForwardRule node) {
1115+
_forwardedUrls.add(importCache
1116+
.canonicalize(node.url, baseImporter: importer, baseUrl: currentUrl)
1117+
?.item2 ??
1118+
node.url);
1119+
}
11041120

11051121
/// Adds a namespace to any variable that requires it.
11061122
@override
@@ -1116,7 +1132,7 @@ class _ModuleMigrationVisitor extends MigrationVisitor {
11161132
if (namespace != null) {
11171133
// Surround the variable in parens if negated to avoid `-` being parsed
11181134
// as part of the namespace.
1119-
var negated = matchesBeforeSpan(node.span, '-');
1135+
var negated = node.span.matchesBefore('-');
11201136
if (negated) addPatch(patchBefore(node, '('));
11211137
addPatch(patchBefore(node, '$namespace.'));
11221138
if (negated) addPatch(patchAfter(node, ')'));
@@ -1215,7 +1231,6 @@ class _ModuleMigrationVisitor extends MigrationVisitor {
12151231
if (libraryUrls != null) {
12161232
url = minBy(libraryUrls, (url) => url.pathSegments.length);
12171233
}
1218-
12191234
if (!_usedUrls.contains(url)) {
12201235
// Add new `@use` rule for indirect dependency
12211236
var tuple = _absoluteUrlToDependency(url);

lib/src/migrators/namespace.dart

+1-1
Original file line numberDiff line numberDiff line change
@@ -157,7 +157,7 @@ class _NamespaceMigrationVisitor extends MigrationVisitor {
157157
assert(span.text.startsWith(namespace));
158158
_spansByNamespace
159159
.putIfAbsent(namespace, () => {})
160-
.add(subspan(span, end: namespace.length));
160+
.add(span.subspan(0, namespace.length));
161161
}
162162
}
163163

lib/src/utils.dart

+46-62
Original file line numberDiff line numberDiff line change
@@ -17,6 +17,43 @@ import 'package:sass/src/ast/node.dart';
1717
import 'io.dart';
1818
import 'patch.dart';
1919

20+
extension ExtendSpan on FileSpan {
21+
/// Extends this span so it encompasses any whitespace on either side of it.
22+
FileSpan extendThroughWhitespace() {
23+
var text = file.getText(0);
24+
25+
var newStart = start.offset - 1;
26+
for (; newStart >= 0; newStart--) {
27+
if (!isWhitespace(text.codeUnitAt(newStart))) break;
28+
}
29+
30+
var newEnd = end.offset;
31+
for (; newEnd < text.length; newEnd++) {
32+
if (!isWhitespace(text.codeUnitAt(newEnd))) break;
33+
}
34+
35+
// Add 1 to start because it's guaranteed to end on either -1 or a character
36+
// that's not whitespace.
37+
return file.span(newStart + 1, newEnd);
38+
}
39+
40+
/// Extends this span forward if it's followed by exactly [pattern].
41+
///
42+
/// If it doesn't match, returns the span as-is.
43+
FileSpan extendIfMatches(Pattern pattern) {
44+
var text = file.getText(end.offset);
45+
var match = pattern.matchAsPrefix(text);
46+
if (match == null) return this;
47+
return file.span(start.offset, end.offset + match.end);
48+
}
49+
50+
/// Returns true if this span is preceded by exactly [text].
51+
bool matchesBefore(String text) {
52+
if (start.offset - text.length < 0) return false;
53+
return file.getText(start.offset - text.length, start.offset) == text;
54+
}
55+
}
56+
2057
/// Returns the default namespace for a use rule with [path].
2158
String namespaceForPath(String path) {
2259
// TODO(jathak): Confirm that this is a valid Sass identifier
@@ -41,40 +78,7 @@ bool valuesAreUnique(Map<Object, Object> map) =>
4178
/// By default, this deletes the entire span. If [start] and/or [end] are
4279
/// provided, this deletes only the portion of the span within that range.
4380
Patch patchDelete(FileSpan span, {int start = 0, int end}) =>
44-
Patch(subspan(span, start: start, end: end), "");
45-
46-
/// Returns a subsection of [span].
47-
FileSpan subspan(FileSpan span, {int start = 0, int end}) => span.file
48-
.span(span.start.offset + start, span.start.offset + (end ?? span.length));
49-
50-
/// Extends [span] so it encompasses any whitespace on either side of it.
51-
FileSpan extendThroughWhitespace(FileSpan span) {
52-
var text = span.file.getText(0);
53-
54-
var start = span.start.offset - 1;
55-
for (; start >= 0; start--) {
56-
if (!isWhitespace(text.codeUnitAt(start))) break;
57-
}
58-
59-
var end = span.end.offset;
60-
for (; end < text.length; end++) {
61-
if (!isWhitespace(text.codeUnitAt(end))) break;
62-
}
63-
64-
// Add 1 to start because it's guaranteed to end on either -1 or a character
65-
// that's not whitespace.
66-
return span.file.span(start + 1, end);
67-
}
68-
69-
/// Extends [span] forward if it's followed by exactly [text].
70-
///
71-
/// If [span] is followed by anything other than [text], returns `null`.
72-
FileSpan extendForward(FileSpan span, String text) {
73-
var end = span.end.offset;
74-
if (end + text.length > span.file.length) return null;
75-
if (span.file.getText(end, end + text.length) != text) return null;
76-
return span.file.span(span.start.offset, end + text.length);
77-
}
81+
Patch(span.subspan(start, end), "");
7882

7983
/// Returns the next location after [import] that it would be safe to insert
8084
/// a `@use` or `@forward` rule.
@@ -117,23 +121,6 @@ FileLocation afterImport(ImportRule import, {bool shouldHaveSemicolon = true}) {
117121
return loc.file.location(loc.offset + i);
118122
}
119123

120-
/// Extends [span] backward if it's preceded by exactly [text].
121-
///
122-
/// If [span] is preceded by anything other than [text], returns `null`.
123-
FileSpan extendBackward(FileSpan span, String text) {
124-
var start = span.start.offset;
125-
if (start - text.length < 0) return null;
126-
if (span.file.getText(start - text.length, start) != text) return null;
127-
return span.file.span(start - text.length, span.end.offset);
128-
}
129-
130-
/// Returns true if [span] is preceded by exactly [text].
131-
bool matchesBeforeSpan(FileSpan span, String text) {
132-
var start = span.start.offset;
133-
if (start - text.length < 0) return false;
134-
return span.file.getText(start - text.length, start) == text;
135-
}
136-
137124
/// Returns whether [character] is whitespace, according to Sass's definition.
138125
bool isWhitespace(int character) =>
139126
character == $space ||
@@ -149,30 +136,27 @@ bool isWhitespace(int character) =>
149136
FileSpan nameSpan(SassNode node) {
150137
if (node is VariableDeclaration) {
151138
var start = node.namespace == null ? 1 : node.namespace.length + 2;
152-
return subspan(node.span, start: start, end: start + node.name.length);
139+
return node.span.subspan(start, start + node.name.length);
153140
} else if (node is VariableExpression) {
154-
return subspan(node.span,
155-
start: node.namespace == null ? 1 : node.namespace.length + 2);
141+
return node.span
142+
.subspan(node.namespace == null ? 1 : node.namespace.length + 2);
156143
} else if (node is FunctionRule) {
157144
var startName = node.span.text
158145
.replaceAll('_', '-')
159146
.indexOf(node.name, '@function'.length);
160-
return subspan(node.span,
161-
start: startName, end: startName + node.name.length);
147+
return node.span.subspan(startName, startName + node.name.length);
162148
} else if (node is FunctionExpression) {
163149
return node.name.span;
164150
} else if (node is MixinRule) {
165151
var startName = node.span.text
166152
.replaceAll('_', '-')
167153
.indexOf(node.name, node.span.text[0] == '=' ? 1 : '@mixin'.length);
168-
return subspan(node.span,
169-
start: startName, end: startName + node.name.length);
154+
return node.span.subspan(startName, startName + node.name.length);
170155
} else if (node is IncludeRule) {
171156
var startName = node.span.text
172157
.replaceAll('_', '-')
173158
.indexOf(node.name, node.span.text[0] == '+' ? 1 : '@include'.length);
174-
return subspan(node.span,
175-
start: startName, end: startName + node.name.length);
159+
return node.span.subspan(startName, startName + node.name.length);
176160
} else {
177161
throw UnsupportedError(
178162
"$node of type ${node.runtimeType} doesn't have a name");
@@ -213,7 +197,7 @@ FileSpan getStaticNameForGetFunctionCall(FunctionExpression node) {
213197
return null;
214198
}
215199
return (nameArgument as StringExpression).hasQuotes
216-
? subspan(nameArgument.span, start: 1, end: nameArgument.span.length - 1)
200+
? nameArgument.span.subspan(1, nameArgument.span.length - 1)
217201
: nameArgument.span;
218202
}
219203

@@ -232,7 +216,7 @@ FileSpan getStaticModuleForGetFunctionCall(FunctionExpression node) {
232216
return null;
233217
}
234218
return (moduleArg as StringExpression).hasQuotes
235-
? subspan(moduleArg.span, start: 1, end: moduleArg.span.length - 2)
219+
? moduleArg.span.subspan(1, moduleArg.span.length - 2)
236220
: moduleArg.span;
237221
}
238222

pubspec.yaml

+2-2
Original file line numberDiff line numberDiff line change
@@ -1,11 +1,11 @@
11
name: sass_migrator
2-
version: 1.3.4
2+
version: 1.3.5
33
description: A tool for running migrations on Sass files
44
author: Jennifer Thakar <[email protected]>
55
homepage: https://github.com/sass/migrator
66

77
environment:
8-
sdk: '>=2.4.0 <3.0.0'
8+
sdk: '>=2.6.0 <3.0.0'
99

1010
dependencies:
1111
args: "^1.5.1"
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,35 @@
1+
<==> arguments
2+
--migrate-deps
3+
4+
<==> input/entrypoint.scss
5+
@import "old";
6+
7+
a {
8+
b: $lib-var1;
9+
c: $lib-var2;
10+
}
11+
12+
<==> input/_old.import.scss
13+
@forward "a.import";
14+
@forward "b.import";
15+
16+
<==> input/_a.scss
17+
$var1: green;
18+
19+
<==> input/_a.import.scss
20+
@forward "a" as lib-*;
21+
22+
<==> input/_b.scss
23+
$var2: blue;
24+
25+
<==> input/_b.import.scss
26+
@forward "b" as lib-*;
27+
28+
<==> output/entrypoint.scss
29+
@use "a";
30+
@use "b";
31+
32+
a {
33+
b: a.$var1;
34+
c: b.$var2;
35+
}

0 commit comments

Comments
 (0)