From faea6f55cfee0b261aec9517d474425089c5cf2b Mon Sep 17 00:00:00 2001 From: Jennifer Thakar Date: Fri, 12 Jul 2024 16:13:16 -0700 Subject: [PATCH 1/4] Add --builtin-only flag to module migrator Also fixes some built-in functions that weren't being migrated. --- CHANGELOG.md | 10 ++++ lib/src/migration_visitor.dart | 2 +- lib/src/migrators/calc_interpolation.dart | 9 ++- lib/src/migrators/division.dart | 9 +-- lib/src/migrators/module.dart | 58 ++++++++++++++----- .../migrators/module/built_in_functions.dart | 4 ++ lib/src/migrators/namespace.dart | 10 ++-- lib/src/migrators/strict_unary.dart | 7 ++- pubspec.yaml | 4 +- .../built_in_functions/builtin_only.hrx | 19 ++++++ 10 files changed, 96 insertions(+), 36 deletions(-) create mode 100644 test/migrators/module/built_in_functions/builtin_only.hrx diff --git a/CHANGELOG.md b/CHANGELOG.md index f2a876b..3474e5d 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -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 diff --git a/lib/src/migration_visitor.dart b/lib/src/migration_visitor.dart index d13de19..53ae6d7 100644 --- a/lib/src/migration_visitor.dart +++ b/lib/src/migration_visitor.dart @@ -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. diff --git a/lib/src/migrators/calc_interpolation.dart b/lib/src/migrators/calc_interpolation.dart index d72891b..a28fe98 100644 --- a/lib/src/migrators/calc_interpolation.dart +++ b/lib/src/migrators/calc_interpolation.dart @@ -19,8 +19,8 @@ class CalculationInterpolationMigrator extends Migrator { @override Map 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; @@ -28,9 +28,8 @@ class CalculationInterpolationMigrator extends Migrator { } class _CalculationInterpolationVisitor extends MigrationVisitor { - _CalculationInterpolationVisitor( - ImportCache importCache, bool migrateDependencies) - : super(importCache, migrateDependencies); + _CalculationInterpolationVisitor(super.importCache, + {required super.migrateDependencies}); @override void visitFunctionExpression(FunctionExpression node) { diff --git a/lib/src/migrators/division.dart b/lib/src/migrators/division.dart index 39827e8..1f8a90b 100644 --- a/lib/src/migrators/division.dart +++ b/lib/src/migrators/division.dart @@ -66,7 +66,8 @@ More info: https://sass-lang.com/d/slash-div"""; Map 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; @@ -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; diff --git a/lib/src/migrators/module.dart b/lib/src/migrators/module.dart index e1afbf8..e15b2aa 100644 --- a/lib/src/migrators/module.dart +++ b/lib/src/migrators/module.dart @@ -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' @@ -77,6 +79,13 @@ class ModuleMigrator extends Migrator { Map 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( @@ -85,8 +94,10 @@ class ModuleMigrator extends Migrator { } var references = References(importCache, stylesheet, importer); - var visitor = _ModuleMigrationVisitor(importCache, references, - globalResults!['load-path'] as List, migrateDependencies, + var visitor = _ModuleMigrationVisitor( + importCache, references, globalResults!['load-path'] as List, + migrateDependencies: migrateDependencies, + builtInOnly: builtInOnly, prefixesToRemove: (argResults!['remove-prefix'] as List) .map((prefix) => prefix.replaceAll('_', '-')), forwards: forwards); @@ -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 loadPaths; @@ -199,6 +207,9 @@ class _ModuleMigrationVisitor extends MigrationVisitor { /// The values of the --forward flag. final Set 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]. @@ -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 loadPaths, bool migrateDependencies, - {Iterable prefixesToRemove = const [], this.forwards = const {}}) + _ModuleMigrationVisitor( + super.importCache, this.references, List loadPaths, + {required super.migrateDependencies, + required this.builtInOnly, + Iterable 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 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); @@ -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; } @@ -613,7 +628,7 @@ class _ModuleMigrationVisitor extends MigrationVisitor { }); } - if (node.name == "get-function") { + if (node.name == "get-function" && !builtInOnly) { var declaration = references.getFunctionReferences[node]; if (declaration != null) { _unreferencable.check(declaration, node); @@ -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); + return; + } else if (_useAllowed) { migratedRules.addAll(_migrateImportToRules(ruleUrl, import.span)); } else { migratedRules.add(_migrateImportToLoadCss(ruleUrl, import.span) @@ -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) { @@ -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); } @@ -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. @@ -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]; diff --git a/lib/src/migrators/module/built_in_functions.dart b/lib/src/migrators/module/built_in_functions.dart index 72aaf00..b17917c 100644 --- a/lib/src/migrators/module/built_in_functions.dart +++ b/lib/src/migrators/module/built_in_functions.dart @@ -22,6 +22,7 @@ const builtInFunctionModules = { "complement": "color", "invert": "color", "alpha": "color", + "opacity": "color", "opacify": "color", "fade-in": "color", "transparentize": "color", @@ -43,6 +44,7 @@ const builtInFunctionModules = { "is-superselector": "selector", "simple-selectors": "selector", "selector-parse": "selector", + "selector-extend": "selector", "percentage": "math", "round": "math", "ceil": "math", @@ -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", @@ -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", diff --git a/lib/src/migrators/namespace.dart b/lib/src/migrators/namespace.dart index 39c1efd..3ffc21d 100644 --- a/lib/src/migrators/namespace.dart +++ b/lib/src/migrators/namespace.dart @@ -38,8 +38,9 @@ class NamespaceMigrator extends Migrator { var renamer = Renamer(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; @@ -62,9 +63,8 @@ class _NamespaceMigrationVisitor extends MigrationVisitor { assertInStylesheet(__usedNamespaces, '_usedNamespaces'); Set? __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) { diff --git a/lib/src/migrators/strict_unary.dart b/lib/src/migrators/strict_unary.dart index 2dc73a0..0d426dd 100644 --- a/lib/src/migrators/strict_unary.dart +++ b/lib/src/migrators/strict_unary.dart @@ -19,7 +19,8 @@ class StrictUnaryMigrator extends Migrator { @override Map 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; @@ -27,8 +28,8 @@ class StrictUnaryMigrator extends Migrator { } class _UnaryMigrationVisitor extends MigrationVisitor { - _UnaryMigrationVisitor(ImportCache importCache, bool migrateDependencies) - : super(importCache, migrateDependencies); + _UnaryMigrationVisitor(super.importCache, + {required super.migrateDependencies}); @override void visitBinaryOperationExpression(BinaryOperationExpression node) { diff --git a/pubspec.yaml b/pubspec.yaml index df61c11..07e3b2f 100644 --- a/pubspec.yaml +++ b/pubspec.yaml @@ -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 @@ -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 diff --git a/test/migrators/module/built_in_functions/builtin_only.hrx b/test/migrators/module/built_in_functions/builtin_only.hrx new file mode 100644 index 0000000..f1af697 --- /dev/null +++ b/test/migrators/module/built_in_functions/builtin_only.hrx @@ -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); +} From 988a53f37898aad8cec37cbcf8199d06be52c6c6 Mon Sep 17 00:00:00 2001 From: Jennifer Thakar Date: Wed, 17 Jul 2024 13:32:33 -0700 Subject: [PATCH 2/4] Fix broken calc_interpolation test --- .../calc_interpolation/calc_remove_interpolation.hrx | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/test/migrators/calc_interpolation/calc_remove_interpolation.hrx b/test/migrators/calc_interpolation/calc_remove_interpolation.hrx index 0e0767b..ef28802 100644 --- a/test/migrators/calc_interpolation/calc_remove_interpolation.hrx +++ b/test/migrators/calc_interpolation/calc_remove_interpolation.hrx @@ -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}); } @@ -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 From ddbf9d60e3991d97bff1466fb3608859596bcf5d Mon Sep 17 00:00:00 2001 From: Jennifer Thakar Date: Wed, 9 Oct 2024 11:25:18 -0700 Subject: [PATCH 3/4] Add more tests; fix issue with null namespace use --- lib/src/migrators/module.dart | 6 ++-- lib/src/migrators/module/references.dart | 12 ++++++-- .../basic.hrx} | 0 .../builtin_only/local_shadow.hrx | 20 +++++++++++++ .../builtin_only/migrate_deps.hrx | 24 +++++++++++++++ .../builtin_only/module_already_exists.hrx | 20 +++++++++++++ .../builtin_only/multiple_imports.hrx | 29 +++++++++++++++++++ .../builtin_only/partially_migrated.hrx | 19 ++++++++++++ .../builtin_only/shadowed_by_import.hrx | 20 +++++++++++++ .../builtin_only/shadowed_by_use.hrx | 22 ++++++++++++++ .../built_in_functions/shadowed_by_use.hrx | 24 +++++++++++++++ .../partial_migration/use_null_namespace.hrx | 20 +++++++++++++ 12 files changed, 211 insertions(+), 5 deletions(-) rename test/migrators/module/built_in_functions/{builtin_only.hrx => builtin_only/basic.hrx} (100%) create mode 100644 test/migrators/module/built_in_functions/builtin_only/local_shadow.hrx create mode 100644 test/migrators/module/built_in_functions/builtin_only/migrate_deps.hrx create mode 100644 test/migrators/module/built_in_functions/builtin_only/module_already_exists.hrx create mode 100644 test/migrators/module/built_in_functions/builtin_only/multiple_imports.hrx create mode 100644 test/migrators/module/built_in_functions/builtin_only/partially_migrated.hrx create mode 100644 test/migrators/module/built_in_functions/builtin_only/shadowed_by_import.hrx create mode 100644 test/migrators/module/built_in_functions/builtin_only/shadowed_by_use.hrx create mode 100644 test/migrators/module/built_in_functions/shadowed_by_use.hrx create mode 100644 test/migrators/module/partial_migration/use_null_namespace.hrx diff --git a/lib/src/migrators/module.dart b/lib/src/migrators/module.dart index e15b2aa..26eb154 100644 --- a/lib/src/migrators/module.dart +++ b/lib/src/migrators/module.dart @@ -611,8 +611,10 @@ class _ModuleMigrationVisitor extends MigrationVisitor { /// Adds a namespace to any function call that requires it. @override void visitFunctionExpression(FunctionExpression node) { + var source = references.sources[node]; if (node.namespace != null || - (builtInOnly && references.sources[node] is! BuiltInSource)) { + source is UseSource || + builtInOnly && source is! BuiltInSource) { super.visitFunctionExpression(node); return; } @@ -795,7 +797,6 @@ class _ModuleMigrationVisitor extends MigrationVisitor { _upstreamStylesheets.add(currentUrl); if (migrateDependencies) visitDependency(ruleUrl, import.span); _upstreamStylesheets.remove(currentUrl); - return; } else if (_useAllowed) { migratedRules.addAll(_migrateImportToRules(ruleUrl, import.span)); } else { @@ -804,6 +805,7 @@ class _ModuleMigrationVisitor extends MigrationVisitor { } } } + if (builtInOnly) return; rulesText = migratedRules.join('$semicolon\n$indent'); if (rulesText.isEmpty) { diff --git a/lib/src/migrators/module/references.dart b/lib/src/migrators/module/references.dart index c65d293..1f3619e 100644 --- a/lib/src/migrators/module/references.dart +++ b/lib/src/migrators/module/references.dart @@ -383,13 +383,19 @@ class _ReferenceVisitor extends ScopedAstVisitor { void visitUseRule(UseRule node) { super.visitUseRule(node); var namespace = node.namespace; - if (namespace == null) return; if (node.url.scheme == 'sass') { - _namespaces[namespace] = node.url; + if (namespace != null) _namespaces[namespace] = node.url; return; } var canonicalUrl = _loadUseOrForward(node.url, node); - _namespaces[namespace] = canonicalUrl; + if (namespace != null) { + _namespaces[namespace] = canonicalUrl; + } else { + var moduleScope = _moduleScopes[canonicalUrl]!; + currentScope.variables.addAll(moduleScope.variables); + currentScope.mixins.addAll(moduleScope.mixins); + currentScope.functions.addAll(moduleScope.functions); + } // `_moduleSources[canonicalUrl]` is set in `_loadUseOrForward`. var moduleSources = _moduleSources[canonicalUrl]!; diff --git a/test/migrators/module/built_in_functions/builtin_only.hrx b/test/migrators/module/built_in_functions/builtin_only/basic.hrx similarity index 100% rename from test/migrators/module/built_in_functions/builtin_only.hrx rename to test/migrators/module/built_in_functions/builtin_only/basic.hrx diff --git a/test/migrators/module/built_in_functions/builtin_only/local_shadow.hrx b/test/migrators/module/built_in_functions/builtin_only/local_shadow.hrx new file mode 100644 index 0000000..7b62b0f --- /dev/null +++ b/test/migrators/module/built_in_functions/builtin_only/local_shadow.hrx @@ -0,0 +1,20 @@ +<==> arguments +--builtin-only + +<==> input/entrypoint.scss +@import "library"; + +@function mix($a, $b) { + @return $a; +} + +a { + color: mix($red, $blue); +} + +<==> input/_library.scss +$red: red; +$blue: blue; + +<==> log.txt +Nothing to migrate! diff --git a/test/migrators/module/built_in_functions/builtin_only/migrate_deps.hrx b/test/migrators/module/built_in_functions/builtin_only/migrate_deps.hrx new file mode 100644 index 0000000..0c2d538 --- /dev/null +++ b/test/migrators/module/built_in_functions/builtin_only/migrate_deps.hrx @@ -0,0 +1,24 @@ +<==> arguments +--builtin-only --migrate-deps + +<==> input/entrypoint.scss +@import "library"; +a { + color: mix($red, $blue); +} + +<==> input/_library.scss +$red: invert(red); +$blue: invert(blue); + +<==> output/entrypoint.scss +@use "sass:color"; +@import "library"; +a { + color: color.mix($red, $blue); +} + +<==> output/_library.scss +@use "sass:color"; +$red: color.invert(red); +$blue: color.invert(blue); diff --git a/test/migrators/module/built_in_functions/builtin_only/module_already_exists.hrx b/test/migrators/module/built_in_functions/builtin_only/module_already_exists.hrx new file mode 100644 index 0000000..f08a2fc --- /dev/null +++ b/test/migrators/module/built_in_functions/builtin_only/module_already_exists.hrx @@ -0,0 +1,20 @@ +<==> arguments +--builtin-only + +<==> input/entrypoint.scss +@use "sass:color"; +@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); +} diff --git a/test/migrators/module/built_in_functions/builtin_only/multiple_imports.hrx b/test/migrators/module/built_in_functions/builtin_only/multiple_imports.hrx new file mode 100644 index 0000000..6a68114 --- /dev/null +++ b/test/migrators/module/built_in_functions/builtin_only/multiple_imports.hrx @@ -0,0 +1,29 @@ +<==> arguments +--builtin-only --migrate-deps + +<==> input/entrypoint.scss +@import "a", "b"; +a { + color: mix($red, $blue); +} + +<==> input/_a.scss +$red: invert(red); + +<==> input/_b.scss +$blue: invert(blue); + +<==> output/entrypoint.scss +@use "sass:color"; +@import "a", "b"; +a { + color: color.mix($red, $blue); +} + +<==> output/_a.scss +@use "sass:color"; +$red: color.invert(red); + +<==> output/_b.scss +@use "sass:color"; +$blue: color.invert(blue); diff --git a/test/migrators/module/built_in_functions/builtin_only/partially_migrated.hrx b/test/migrators/module/built_in_functions/builtin_only/partially_migrated.hrx new file mode 100644 index 0000000..0e60b37 --- /dev/null +++ b/test/migrators/module/built_in_functions/builtin_only/partially_migrated.hrx @@ -0,0 +1,19 @@ +<==> arguments +--builtin-only + +<==> input/entrypoint.scss +@use "sass:math"; +@import "library"; +a { + b: round(math.sqrt($c)); +} + +<==> input/_library.scss +$c: 7; + +<==> output/entrypoint.scss +@use "sass:math"; +@import "library"; +a { + b: math.round(math.sqrt($c)); +} diff --git a/test/migrators/module/built_in_functions/builtin_only/shadowed_by_import.hrx b/test/migrators/module/built_in_functions/builtin_only/shadowed_by_import.hrx new file mode 100644 index 0000000..493f55c --- /dev/null +++ b/test/migrators/module/built_in_functions/builtin_only/shadowed_by_import.hrx @@ -0,0 +1,20 @@ +<==> arguments +--builtin-only + +<==> input/entrypoint.scss +@import "library"; + +a { + color: mix($red, $blue); +} + +<==> input/_library.scss +$red: red; +$blue: blue; + +@function mix($a, $b) { + @return $a; +} + +<==> log.txt +Nothing to migrate! diff --git a/test/migrators/module/built_in_functions/builtin_only/shadowed_by_use.hrx b/test/migrators/module/built_in_functions/builtin_only/shadowed_by_use.hrx new file mode 100644 index 0000000..7ff1441 --- /dev/null +++ b/test/migrators/module/built_in_functions/builtin_only/shadowed_by_use.hrx @@ -0,0 +1,22 @@ +<==> arguments +--builtin-only + +<==> input/entrypoint.scss +@use "library" as *; +@import "colors"; + +a { + color: mix($red, $blue); +} + +<==> input/_library.scss +@function mix($a, $b) { + @return $a; +} + +<==> input/_colors.scss +$red: red; +$blue: blue; + +<==> log.txt +Nothing to migrate! diff --git a/test/migrators/module/built_in_functions/shadowed_by_use.hrx b/test/migrators/module/built_in_functions/shadowed_by_use.hrx new file mode 100644 index 0000000..908770b --- /dev/null +++ b/test/migrators/module/built_in_functions/shadowed_by_use.hrx @@ -0,0 +1,24 @@ +<==> input/entrypoint.scss +@use "library" as *; +@import "colors"; + +a { + color: mix($red, $blue); +} + +<==> input/_library.scss +@function mix($a, $b) { + @return $a; +} + +<==> input/_colors.scss +$red: red; +$blue: blue; + +<==> output/entrypoint.scss +@use "library" as *; +@use "colors"; + +a { + color: mix(colors.$red, colors.$blue); +} diff --git a/test/migrators/module/partial_migration/use_null_namespace.hrx b/test/migrators/module/partial_migration/use_null_namespace.hrx new file mode 100644 index 0000000..1bd44a6 --- /dev/null +++ b/test/migrators/module/partial_migration/use_null_namespace.hrx @@ -0,0 +1,20 @@ +<==> input/entrypoint.scss +@use "library" as *; + +a { + @include test(fn($x)); +} + +<==> input/_library.scss +@mixin test($arg) { + b: $arg; +} + +@function fn($arg) { + @return $arg; +} + +$x: red; + +<==> log.txt +Nothing to migrate! From 9fe3c661daea5a0a00058f188fa086f19bf66150 Mon Sep 17 00:00:00 2001 From: Jennifer Thakar Date: Thu, 10 Oct 2024 10:41:53 -0700 Subject: [PATCH 4/4] Code review --- CHANGELOG.md | 2 +- lib/src/migrators/module.dart | 21 ++++++++++------ .../{builtin_only => built_in_only}/basic.hrx | 2 +- .../built_in_only/get_function.hrx | 25 +++++++++++++++++++ .../local_shadow.hrx | 2 +- .../migrate_deps.hrx | 2 +- .../module_already_exists.hrx | 2 +- .../multiple_imports.hrx | 2 +- .../partially_migrated.hrx | 2 +- .../shadowed_by_import.hrx | 2 +- .../shadowed_by_use.hrx | 2 +- .../module/built_in_functions/namespacing.hrx | 2 ++ 12 files changed, 49 insertions(+), 17 deletions(-) rename test/migrators/module/built_in_functions/{builtin_only => built_in_only}/basic.hrx (94%) create mode 100644 test/migrators/module/built_in_functions/built_in_only/get_function.hrx rename test/migrators/module/built_in_functions/{builtin_only => built_in_only}/local_shadow.hrx (93%) rename test/migrators/module/built_in_functions/{builtin_only => built_in_only}/migrate_deps.hrx (92%) rename test/migrators/module/built_in_functions/{builtin_only => built_in_only}/module_already_exists.hrx (94%) rename test/migrators/module/built_in_functions/{builtin_only => built_in_only}/multiple_imports.hrx (92%) rename test/migrators/module/built_in_functions/{builtin_only => built_in_only}/partially_migrated.hrx (93%) rename test/migrators/module/built_in_functions/{builtin_only => built_in_only}/shadowed_by_import.hrx (93%) rename test/migrators/module/built_in_functions/{builtin_only => built_in_only}/shadowed_by_use.hrx (94%) diff --git a/CHANGELOG.md b/CHANGELOG.md index 2392b8b..1dacf10 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -2,7 +2,7 @@ ### Module Migrator -* Add a new `--builtin-only` flag, which migrates global functions to their +* Add a new `--built-in-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 diff --git a/lib/src/migrators/module.dart b/lib/src/migrators/module.dart index 26eb154..ba623ec 100644 --- a/lib/src/migrators/module.dart +++ b/lib/src/migrators/module.dart @@ -32,7 +32,7 @@ class ModuleMigrator extends Migrator { @override final argParser = ArgParser() - ..addFlag('builtin-only', + ..addFlag('built-in-only', help: 'Migrates global functions without migrating @import.') ..addMultiOption('remove-prefix', abbr: 'p', @@ -79,12 +79,12 @@ class ModuleMigrator extends Migrator { Map migrateFile( ImportCache importCache, Stylesheet stylesheet, Importer importer) { var forwards = {for (var arg in argResults!['forward']) ForwardType(arg)}; - var builtInOnly = argResults!['builtin-only'] as bool; + var builtInOnly = argResults!['built-in-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.'); + 'passed with --built-in-only.'); } if (forwards.contains(ForwardType.prefixed) && !argResults!.wasParsed('remove-prefix')) { @@ -630,7 +630,7 @@ class _ModuleMigrationVisitor extends MigrationVisitor { }); } - if (node.name == "get-function" && !builtInOnly) { + if (node.name == "get-function") { var declaration = references.getFunctionReferences[node]; if (declaration != null) { _unreferencable.check(declaration, node); @@ -648,12 +648,15 @@ class _ModuleMigrationVisitor extends MigrationVisitor { // Warn for get-function calls without a static name. var nameArg = node.arguments.named['name'] ?? node.arguments.positional.first; - if (nameArg is! StringExpression || nameArg.text.asPlain == null) { + if ((nameArg is! StringExpression || nameArg.text.asPlain == null) && + !builtInOnly) { emitWarning( "get-function call may require \$module parameter", nameArg.span); return; } + if (builtInOnly && declaration != null) return; + _patchNamespaceForFunction(node, declaration, (namespace) { var beforeParen = node.span.end.offset - 1; addPatch(Patch(node.span.file.span(beforeParen, beforeParen), @@ -794,9 +797,11 @@ class _ModuleMigrationVisitor extends MigrationVisitor { } if (ruleUrl != null) { if (builtInOnly) { - _upstreamStylesheets.add(currentUrl); - if (migrateDependencies) visitDependency(ruleUrl, import.span); - _upstreamStylesheets.remove(currentUrl); + if (migrateDependencies) { + _upstreamStylesheets.add(currentUrl); + visitDependency(ruleUrl, import.span); + _upstreamStylesheets.remove(currentUrl); + } } else if (_useAllowed) { migratedRules.addAll(_migrateImportToRules(ruleUrl, import.span)); } else { diff --git a/test/migrators/module/built_in_functions/builtin_only/basic.hrx b/test/migrators/module/built_in_functions/built_in_only/basic.hrx similarity index 94% rename from test/migrators/module/built_in_functions/builtin_only/basic.hrx rename to test/migrators/module/built_in_functions/built_in_only/basic.hrx index f1af697..7ae2ad6 100644 --- a/test/migrators/module/built_in_functions/builtin_only/basic.hrx +++ b/test/migrators/module/built_in_functions/built_in_only/basic.hrx @@ -1,5 +1,5 @@ <==> arguments ---builtin-only +--built-in-only <==> input/entrypoint.scss @import "library"; diff --git a/test/migrators/module/built_in_functions/built_in_only/get_function.hrx b/test/migrators/module/built_in_functions/built_in_only/get_function.hrx new file mode 100644 index 0000000..62d9546 --- /dev/null +++ b/test/migrators/module/built_in_functions/built_in_only/get_function.hrx @@ -0,0 +1,25 @@ +<==> arguments +--built-in-only + +<==> input/entrypoint.scss +@import "library"; + +a { + b: call(get-function(mix), red, blue); + c: call(get-function(fn), green); +} + +<==> input/_library.scss +@function fn($x) { + @return $x; +} + +<==> output/entrypoint.scss +@use "sass:color"; +@use "sass:meta"; +@import "library"; + +a { + b: meta.call(meta.get-function(mix, $module: "color"), red, blue); + c: meta.call(meta.get-function(fn), green); +} diff --git a/test/migrators/module/built_in_functions/builtin_only/local_shadow.hrx b/test/migrators/module/built_in_functions/built_in_only/local_shadow.hrx similarity index 93% rename from test/migrators/module/built_in_functions/builtin_only/local_shadow.hrx rename to test/migrators/module/built_in_functions/built_in_only/local_shadow.hrx index 7b62b0f..d417c2d 100644 --- a/test/migrators/module/built_in_functions/builtin_only/local_shadow.hrx +++ b/test/migrators/module/built_in_functions/built_in_only/local_shadow.hrx @@ -1,5 +1,5 @@ <==> arguments ---builtin-only +--built-in-only <==> input/entrypoint.scss @import "library"; diff --git a/test/migrators/module/built_in_functions/builtin_only/migrate_deps.hrx b/test/migrators/module/built_in_functions/built_in_only/migrate_deps.hrx similarity index 92% rename from test/migrators/module/built_in_functions/builtin_only/migrate_deps.hrx rename to test/migrators/module/built_in_functions/built_in_only/migrate_deps.hrx index 0c2d538..7a53fa2 100644 --- a/test/migrators/module/built_in_functions/builtin_only/migrate_deps.hrx +++ b/test/migrators/module/built_in_functions/built_in_only/migrate_deps.hrx @@ -1,5 +1,5 @@ <==> arguments ---builtin-only --migrate-deps +--built-in-only --migrate-deps <==> input/entrypoint.scss @import "library"; diff --git a/test/migrators/module/built_in_functions/builtin_only/module_already_exists.hrx b/test/migrators/module/built_in_functions/built_in_only/module_already_exists.hrx similarity index 94% rename from test/migrators/module/built_in_functions/builtin_only/module_already_exists.hrx rename to test/migrators/module/built_in_functions/built_in_only/module_already_exists.hrx index f08a2fc..c03c437 100644 --- a/test/migrators/module/built_in_functions/builtin_only/module_already_exists.hrx +++ b/test/migrators/module/built_in_functions/built_in_only/module_already_exists.hrx @@ -1,5 +1,5 @@ <==> arguments ---builtin-only +--built-in-only <==> input/entrypoint.scss @use "sass:color"; diff --git a/test/migrators/module/built_in_functions/builtin_only/multiple_imports.hrx b/test/migrators/module/built_in_functions/built_in_only/multiple_imports.hrx similarity index 92% rename from test/migrators/module/built_in_functions/builtin_only/multiple_imports.hrx rename to test/migrators/module/built_in_functions/built_in_only/multiple_imports.hrx index 6a68114..49c91a2 100644 --- a/test/migrators/module/built_in_functions/builtin_only/multiple_imports.hrx +++ b/test/migrators/module/built_in_functions/built_in_only/multiple_imports.hrx @@ -1,5 +1,5 @@ <==> arguments ---builtin-only --migrate-deps +--built-in-only --migrate-deps <==> input/entrypoint.scss @import "a", "b"; diff --git a/test/migrators/module/built_in_functions/builtin_only/partially_migrated.hrx b/test/migrators/module/built_in_functions/built_in_only/partially_migrated.hrx similarity index 93% rename from test/migrators/module/built_in_functions/builtin_only/partially_migrated.hrx rename to test/migrators/module/built_in_functions/built_in_only/partially_migrated.hrx index 0e60b37..12162c7 100644 --- a/test/migrators/module/built_in_functions/builtin_only/partially_migrated.hrx +++ b/test/migrators/module/built_in_functions/built_in_only/partially_migrated.hrx @@ -1,5 +1,5 @@ <==> arguments ---builtin-only +--built-in-only <==> input/entrypoint.scss @use "sass:math"; diff --git a/test/migrators/module/built_in_functions/builtin_only/shadowed_by_import.hrx b/test/migrators/module/built_in_functions/built_in_only/shadowed_by_import.hrx similarity index 93% rename from test/migrators/module/built_in_functions/builtin_only/shadowed_by_import.hrx rename to test/migrators/module/built_in_functions/built_in_only/shadowed_by_import.hrx index 493f55c..266f60a 100644 --- a/test/migrators/module/built_in_functions/builtin_only/shadowed_by_import.hrx +++ b/test/migrators/module/built_in_functions/built_in_only/shadowed_by_import.hrx @@ -1,5 +1,5 @@ <==> arguments ---builtin-only +--built-in-only <==> input/entrypoint.scss @import "library"; diff --git a/test/migrators/module/built_in_functions/builtin_only/shadowed_by_use.hrx b/test/migrators/module/built_in_functions/built_in_only/shadowed_by_use.hrx similarity index 94% rename from test/migrators/module/built_in_functions/builtin_only/shadowed_by_use.hrx rename to test/migrators/module/built_in_functions/built_in_only/shadowed_by_use.hrx index 7ff1441..3038439 100644 --- a/test/migrators/module/built_in_functions/builtin_only/shadowed_by_use.hrx +++ b/test/migrators/module/built_in_functions/built_in_only/shadowed_by_use.hrx @@ -1,5 +1,5 @@ <==> arguments ---builtin-only +--built-in-only <==> input/entrypoint.scss @use "library" as *; diff --git a/test/migrators/module/built_in_functions/namespacing.hrx b/test/migrators/module/built_in_functions/namespacing.hrx index 7cf86d4..5904584 100644 --- a/test/migrators/module/built_in_functions/namespacing.hrx +++ b/test/migrators/module/built_in_functions/namespacing.hrx @@ -2,6 +2,7 @@ $a: mix(red, blue); $b: str-length("hello"); $c: scale-color(blue, $lightness: -10%); +$d: call(get-function(mix), red, blue); @function fn($args...) { @return keywords($args); @@ -14,6 +15,7 @@ $c: scale-color(blue, $lightness: -10%); $a: color.mix(red, blue); $b: string.length("hello"); $c: color.scale(blue, $lightness: -10%); +$d: meta.call(meta.get-function(mix, $module: "color"), red, blue); @function fn($args...) { @return meta.keywords($args);