Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add --built-in-only flag to module migrator #259

Merged
merged 5 commits into from
Oct 11, 2024
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 10 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
@@ -1,3 +1,13 @@
## 2.1.0

### Module Migrator

* Add a new `--builtin-only` flag, which migrates global functions to their
`sass:` module equivalents, while leaving `@import` rules unchanged.

* Fix bug where some functions (`opacity`, `is-bracketed`, and
`selector-extend`) would not be migrated.

## 2.0.3

### Module Migrator
Expand Down
2 changes: 1 addition & 1 deletion lib/src/migration_visitor.dart
Original file line number Diff line number Diff line change
Expand Up @@ -66,7 +66,7 @@ abstract class MigrationVisitor extends ScopedAstVisitor {
/// syntax, in which case this returns an empty string.
String get semicolon => currentUrl.path.endsWith('.sass') ? "" : ";";

MigrationVisitor(this.importCache, this.migrateDependencies);
MigrationVisitor(this.importCache, {required this.migrateDependencies});

/// Runs a new migration on [stylesheet] (and its dependencies, if
/// [migrateDependencies] is true) and returns a map of migrated contents.
Expand Down
9 changes: 4 additions & 5 deletions lib/src/migrators/calc_interpolation.dart
Original file line number Diff line number Diff line change
Expand Up @@ -19,18 +19,17 @@ class CalculationInterpolationMigrator extends Migrator {
@override
Map<Uri, String> migrateFile(
ImportCache importCache, Stylesheet stylesheet, Importer importer) {
var visitor =
_CalculationInterpolationVisitor(importCache, migrateDependencies);
var visitor = _CalculationInterpolationVisitor(importCache,
migrateDependencies: migrateDependencies);
var result = visitor.run(stylesheet, importer);
missingDependencies.addAll(visitor.missingDependencies);
return result;
}
}

class _CalculationInterpolationVisitor extends MigrationVisitor {
_CalculationInterpolationVisitor(
ImportCache importCache, bool migrateDependencies)
: super(importCache, migrateDependencies);
_CalculationInterpolationVisitor(super.importCache,
{required super.migrateDependencies});

@override
void visitFunctionExpression(FunctionExpression node) {
Expand Down
9 changes: 5 additions & 4 deletions lib/src/migrators/division.dart
Original file line number Diff line number Diff line change
Expand Up @@ -66,7 +66,8 @@ More info: https://sass-lang.com/d/slash-div""";
Map<Uri, String> migrateFile(
ImportCache importCache, Stylesheet stylesheet, Importer importer) {
var visitor = _DivisionMigrationVisitor(
importCache, isPessimistic, useMultiplication, migrateDependencies);
importCache, isPessimistic, useMultiplication,
migrateDependencies: migrateDependencies);
var result = visitor.run(stylesheet, importer);
missingDependencies.addAll(visitor.missingDependencies);
return result;
Expand All @@ -80,9 +81,9 @@ class _DivisionMigrationVisitor extends MigrationVisitor {
final bool isPessimistic;
final bool useMultiplication;

_DivisionMigrationVisitor(ImportCache importCache, this.isPessimistic,
this.useMultiplication, bool migrateDependencies)
: super(importCache, migrateDependencies);
_DivisionMigrationVisitor(
super.importCache, this.isPessimistic, this.useMultiplication,
{required super.migrateDependencies});

/// True when division is allowed by the context the current node is in.
var _isDivisionAllowed = false;
Expand Down
58 changes: 42 additions & 16 deletions lib/src/migrators/module.dart
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,8 @@ class ModuleMigrator extends Migrator {

@override
final argParser = ArgParser()
..addFlag('builtin-only',
help: 'Migrates global functions without migrating @import.')
..addMultiOption('remove-prefix',
abbr: 'p',
help: 'Removes PREFIX from all migrated member names.\n'
Expand Down Expand Up @@ -77,6 +79,13 @@ class ModuleMigrator extends Migrator {
Map<Uri, String> migrateFile(
ImportCache importCache, Stylesheet stylesheet, Importer importer) {
var forwards = {for (var arg in argResults!['forward']) ForwardType(arg)};
var builtInOnly = argResults!['builtin-only'] as bool;
if (builtInOnly &&
(argResults!.wasParsed('forward') ||
argResults!.wasParsed('remove-prefix'))) {
throw MigrationException('--forward and --remove-prefix may not be '
'passed with --builtin-only.');
}
if (forwards.contains(ForwardType.prefixed) &&
!argResults!.wasParsed('remove-prefix')) {
throw MigrationException(
Expand All @@ -85,8 +94,10 @@ class ModuleMigrator extends Migrator {
}

var references = References(importCache, stylesheet, importer);
var visitor = _ModuleMigrationVisitor(importCache, references,
globalResults!['load-path'] as List<String>, migrateDependencies,
var visitor = _ModuleMigrationVisitor(
importCache, references, globalResults!['load-path'] as List<String>,
migrateDependencies: migrateDependencies,
builtInOnly: builtInOnly,
prefixesToRemove: (argResults!['remove-prefix'] as List<String>)
.map((prefix) => prefix.replaceAll('_', '-')),
forwards: forwards);
Expand Down Expand Up @@ -186,9 +197,6 @@ class _ModuleMigrationVisitor extends MigrationVisitor {
/// main migration pass is used.
final References references;

/// Cache used to load stylesheets.
final ImportCache importCache;

/// List of paths that stylesheets can be loaded from.
final List<String> loadPaths;

Expand All @@ -199,6 +207,9 @@ class _ModuleMigrationVisitor extends MigrationVisitor {
/// The values of the --forward flag.
final Set<ForwardType> forwards;

/// Whether to migrate only global functions, leaving `@import` rules as-is.
final bool builtInOnly;

/// Constructs a new module migration visitor.
///
/// [importCache] must be the same one used by [references].
Expand All @@ -210,22 +221,25 @@ class _ModuleMigrationVisitor extends MigrationVisitor {
/// the module migrator will filter out the dependencies' migration results.
///
/// This converts the OS-specific relative [loadPaths] to absolute URL paths.
_ModuleMigrationVisitor(this.importCache, this.references,
List<String> loadPaths, bool migrateDependencies,
{Iterable<String> prefixesToRemove = const [], this.forwards = const {}})
_ModuleMigrationVisitor(
super.importCache, this.references, List<String> loadPaths,
{required super.migrateDependencies,
required this.builtInOnly,
Iterable<String> prefixesToRemove = const [],
this.forwards = const {}})
: loadPaths = List.unmodifiable(
loadPaths.map((path) => p.toUri(p.absolute(path)).path)),
prefixesToRemove = UnmodifiableSetView(prefixesToRemove.toSet()),
super(importCache, migrateDependencies);
prefixesToRemove = UnmodifiableSetView(prefixesToRemove.toSet());

/// Checks which global declarations need to be renamed, then runs the
/// migrator.
@override
Map<Uri, String> run(Stylesheet stylesheet, Importer importer) {
references.globalDeclarations.forEach(_renameDeclaration);
if (!builtInOnly) references.globalDeclarations.forEach(_renameDeclaration);
var migrated = super.run(stylesheet, importer);

if (forwards.contains(ForwardType.importOnly) || _needsImportOnly) {
if (!builtInOnly &&
(forwards.contains(ForwardType.importOnly) || _needsImportOnly)) {
var url = stylesheet.span.sourceUrl!;
var importOnlyUrl = getImportOnlyUrl(url);
var results = _generateImportOnly(url, importOnlyUrl);
Expand Down Expand Up @@ -597,7 +611,8 @@ class _ModuleMigrationVisitor extends MigrationVisitor {
/// Adds a namespace to any function call that requires it.
@override
void visitFunctionExpression(FunctionExpression node) {
if (node.namespace != null) {
if (node.namespace != null ||
(builtInOnly && references.sources[node] is! BuiltInSource)) {
super.visitFunctionExpression(node);
return;
}
Expand All @@ -613,7 +628,7 @@ class _ModuleMigrationVisitor extends MigrationVisitor {
});
}

if (node.name == "get-function") {
if (node.name == "get-function" && !builtInOnly) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What about get-function() calls for functions from built-in modules?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed

var declaration = references.getFunctionReferences[node];
if (declaration != null) {
_unreferencable.check(declaration, node);
Expand Down Expand Up @@ -776,7 +791,12 @@ class _ModuleMigrationVisitor extends MigrationVisitor {
}
}
if (ruleUrl != null) {
if (_useAllowed) {
if (builtInOnly) {
_upstreamStylesheets.add(currentUrl);
if (migrateDependencies) visitDependency(ruleUrl, import.span);
_upstreamStylesheets.remove(currentUrl);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since modifying _upstreamStylesheets doesn't do anything when migrateDependencies is false anyway, this makes it clearer when those lines are relevant:

Suggested change
_upstreamStylesheets.add(currentUrl);
if (migrateDependencies) visitDependency(ruleUrl, import.span);
_upstreamStylesheets.remove(currentUrl);
if (migrateDependencies) {
_upstreamStylesheets.add(currentUrl);
visitDependency(ruleUrl, import.span);
_upstreamStylesheets.remove(currentUrl);
}

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

return;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why return eagerly here? What if there are multiple imports in a single @import that all need to be traversed?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch. Fixed

} else if (_useAllowed) {
migratedRules.addAll(_migrateImportToRules(ruleUrl, import.span));
} else {
migratedRules.add(_migrateImportToLoadCss(ruleUrl, import.span)
Expand Down Expand Up @@ -1071,6 +1091,7 @@ class _ModuleMigrationVisitor extends MigrationVisitor {
_useAllowed = false;
super.visitIncludeRule(node);
if (node.namespace != null) return;
if (builtInOnly && references.sources[node] is! BuiltInSource) return;

var declaration = references.mixins[node];
if (declaration == null) {
Expand All @@ -1089,7 +1110,7 @@ class _ModuleMigrationVisitor extends MigrationVisitor {
@override
void visitMixinRule(MixinRule node) {
_useAllowed = false;
_renameReference(nameSpan(node), MemberDeclaration(node));
if (!builtInOnly) _renameReference(nameSpan(node), MemberDeclaration(node));
super.visitMixinRule(node);
}

Expand Down Expand Up @@ -1119,6 +1140,7 @@ class _ModuleMigrationVisitor extends MigrationVisitor {
@override
void visitVariableExpression(VariableExpression node) {
if (node.namespace != null) return;
if (builtInOnly && references.sources[node] is! BuiltInSource) return;
var declaration = references.variables[node];
if (declaration == null) {
// TODO(jathak): Error here as part of fixing #182.
Expand All @@ -1145,6 +1167,10 @@ class _ModuleMigrationVisitor extends MigrationVisitor {
/// renaming or namespacing if necessary.
@override
void visitVariableDeclaration(VariableDeclaration node) {
if (builtInOnly) {
super.visitVariableDeclaration(node);
return;
}
var declaration = MemberDeclaration(node);
var defaultDeclaration =
references.defaultVariableDeclarations[declaration];
Expand Down
4 changes: 4 additions & 0 deletions lib/src/migrators/module/built_in_functions.dart
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@ const builtInFunctionModules = {
"complement": "color",
"invert": "color",
"alpha": "color",
"opacity": "color",
"opacify": "color",
"fade-in": "color",
"transparentize": "color",
Expand All @@ -43,6 +44,7 @@ const builtInFunctionModules = {
"is-superselector": "selector",
"simple-selectors": "selector",
"selector-parse": "selector",
"selector-extend": "selector",
"percentage": "math",
"round": "math",
"ceil": "math",
Expand All @@ -62,6 +64,7 @@ const builtInFunctionModules = {
"zip": "list",
"index": "list",
"list-separator": "list",
"is-bracketed": "list",
"feature-exists": "meta",
"variable-exists": "meta",
"global-variable-exists": "meta",
Expand Down Expand Up @@ -100,6 +103,7 @@ const builtInFunctionNameChanges = {
"selector-replace": "replace",
"selector-unify": "unify",
"selector-parse": "parse",
"selector-extend": "extend",
"unitless": "is-unitless",
"comparable": "compatible",
"list-separator": "separator",
Expand Down
10 changes: 5 additions & 5 deletions lib/src/migrators/namespace.dart
Original file line number Diff line number Diff line change
Expand Up @@ -38,8 +38,9 @@ class NamespaceMigrator extends Migrator {
var renamer = Renamer<UseRule>(argResults!['rename'].join('\n'),
{'': ((rule) => rule.namespace!), 'url': (rule) => rule.url.toString()},
sourceUrl: '--rename');
var visitor = _NamespaceMigrationVisitor(renamer,
argResults!['force'] as bool, importCache, migrateDependencies);
var visitor = _NamespaceMigrationVisitor(
renamer, argResults!['force'] as bool, importCache,
migrateDependencies: migrateDependencies);
var result = visitor.run(stylesheet, importer);
missingDependencies.addAll(visitor.missingDependencies);
return result;
Expand All @@ -62,9 +63,8 @@ class _NamespaceMigrationVisitor extends MigrationVisitor {
assertInStylesheet(__usedNamespaces, '_usedNamespaces');
Set<String>? __usedNamespaces;

_NamespaceMigrationVisitor(this.renamer, this.forceRename,
ImportCache importCache, bool migrateDependencies)
: super(importCache, migrateDependencies);
_NamespaceMigrationVisitor(this.renamer, this.forceRename, super.importCache,
{required super.migrateDependencies});

@override
void visitStylesheet(Stylesheet node) {
Expand Down
7 changes: 4 additions & 3 deletions lib/src/migrators/strict_unary.dart
Original file line number Diff line number Diff line change
Expand Up @@ -19,16 +19,17 @@ class StrictUnaryMigrator extends Migrator {
@override
Map<Uri, String> migrateFile(
ImportCache importCache, Stylesheet stylesheet, Importer importer) {
var visitor = _UnaryMigrationVisitor(importCache, migrateDependencies);
var visitor = _UnaryMigrationVisitor(importCache,
migrateDependencies: migrateDependencies);
var result = visitor.run(stylesheet, importer);
missingDependencies.addAll(visitor.missingDependencies);
return result;
}
}

class _UnaryMigrationVisitor extends MigrationVisitor {
_UnaryMigrationVisitor(ImportCache importCache, bool migrateDependencies)
: super(importCache, migrateDependencies);
_UnaryMigrationVisitor(super.importCache,
{required super.migrateDependencies});

@override
void visitBinaryOperationExpression(BinaryOperationExpression node) {
Expand Down
4 changes: 2 additions & 2 deletions pubspec.yaml
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
name: sass_migrator
version: 2.0.3
version: 2.1.0
description: A tool for running migrations on Sass files
homepage: https://github.com/sass/migrator

Expand All @@ -17,7 +17,7 @@ dependencies:
node_interop: ^2.0.2
node_io: ^2.3.0
path: ^1.8.0
sass_api: ^9.2.7
sass_api: ^10.4.8
source_span: ^1.8.1
stack_trace: ^1.10.0
string_scanner: ^1.1.0
Expand Down
10 changes: 5 additions & 5 deletions test/migrators/calc_interpolation/calc_remove_interpolation.hrx
Original file line number Diff line number Diff line change
Expand Up @@ -8,8 +8,8 @@ $d: 5;
.a { .b: calc($b * #{$c + 1}); }

// More than one interpolations
.a {
.b: calc($b - #{$c + 1} + #{$d});
.a {
.b: calc($b - #{$c + 1} + #{$d});
.c: calc(100% - #{$TABLE_TITLE + 2px});
}

Expand All @@ -35,9 +35,9 @@ $d: 5;
.a { .b: calc($b * ($c + 1)); }

// More than one interpolations
.a {
.b: calc($b - ($c + 1) + $d);
.c: calc(100% - ($TABLE-TITLE + 2px));
.a {
.b: calc($b - ($c + 1) + $d);
.c: calc(100% - ($TABLE_TITLE + 2px));
}

// Nested
Expand Down
19 changes: 19 additions & 0 deletions test/migrators/module/built_in_functions/builtin_only.hrx
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There are a lot of special cases for builtInOnly in the module migrator. It could probably use a few more tests to exercise more of them.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

Original file line number Diff line number Diff line change
@@ -0,0 +1,19 @@
<==> arguments
--builtin-only

<==> input/entrypoint.scss
@import "library";
a {
color: mix($red, $blue);
}

<==> input/_library.scss
$red: red;
$blue: blue;

<==> output/entrypoint.scss
@use "sass:color";
@import "library";
a {
color: color.mix($red, $blue);
}
Loading