From cf6d3c8a9f6cb17fc7642774a96a4d74d63a846e Mon Sep 17 00:00:00 2001 From: Jennifer Thakar Date: Mon, 1 Apr 2019 10:56:13 -0700 Subject: [PATCH 1/7] Generalizes to support multiple migrators There's now a `Migrator` class that additional migrators can extend. Most migrators will want to extend `MigratorBase`, which handles the tree traversal. For now, `MigratorBase` only operates on a single stylesheet, with the dependency migration implemented only by `ModuleMigrator`. --- ...odule_migrator.dart => sass_migrator.dart} | 46 ++- lib/src/migrator.dart | 378 ++---------------- lib/src/migrator_base.dart | 113 ++++++ lib/src/migrators/module.dart | 302 ++++++++++++++ .../migrators/module/built_in_functions.dart | 102 +++++ lib/src/migrators/module/local_scope.dart | 38 ++ .../module/stylesheet_migration.dart | 85 ++++ lib/src/utils.dart | 21 + pubspec.yaml | 6 +- test/migrations/README.md | 4 +- .../globally_shadowed_variable.hrx | 3 + .../{ => module}/module_configuration.hrx | 3 + .../namespace_builtin_functions.hrx | 0 .../{ => module}/namespace_function.hrx | 3 + .../{ => module}/namespace_get_function.hrx | 3 + .../namespace_indirect_reference.hrx | 3 + .../{ => module}/namespace_mixin.hrx | 3 + .../{ => module}/namespace_variable.hrx | 3 + .../{ => module}/subdirectories.hrx | 3 + test/migrator_test.dart | 36 +- test/patch_test.dart | 2 +- 21 files changed, 785 insertions(+), 372 deletions(-) rename bin/{sass_module_migrator.dart => sass_migrator.dart} (51%) create mode 100644 lib/src/migrator_base.dart create mode 100644 lib/src/migrators/module.dart create mode 100644 lib/src/migrators/module/built_in_functions.dart create mode 100644 lib/src/migrators/module/local_scope.dart create mode 100644 lib/src/migrators/module/stylesheet_migration.dart rename test/migrations/{ => module}/globally_shadowed_variable.hrx (87%) rename test/migrations/{ => module}/module_configuration.hrx (88%) rename test/migrations/{ => module}/namespace_builtin_functions.hrx (100%) rename test/migrations/{ => module}/namespace_function.hrx (87%) rename test/migrations/{ => module}/namespace_get_function.hrx (97%) rename test/migrations/{ => module}/namespace_indirect_reference.hrx (90%) rename test/migrations/{ => module}/namespace_mixin.hrx (87%) rename test/migrations/{ => module}/namespace_variable.hrx (86%) rename test/migrations/{ => module}/subdirectories.hrx (92%) diff --git a/bin/sass_module_migrator.dart b/bin/sass_migrator.dart similarity index 51% rename from bin/sass_module_migrator.dart rename to bin/sass_migrator.dart index 6ebddea0..882629bf 100644 --- a/bin/sass_module_migrator.dart +++ b/bin/sass_migrator.dart @@ -8,32 +8,45 @@ import 'dart:io'; import 'package:args/args.dart'; -import 'package:sass_module_migrator/src/migrator.dart'; +import 'package:sass_migrator/src/migrator.dart'; +import 'package:sass_migrator/src/migrators/module.dart'; + +final migrators = [ModuleMigrator()]; void main(List args) { - var argParser = new ArgParser() + var argParser = ArgParser(usageLineLength: 80) ..addFlag('dry-run', abbr: 'n', help: 'Show which files would be migrated but make no changes.') ..addFlag('verbose', abbr: 'v', help: 'Print text of migrated files when running with --dry-run.') - ..addFlag('help', abbr: 'h', help: 'Print help text.', negatable: false) - ..addFlag('recursive', - abbr: 'r', - help: - 'Migrate all dependencies in addition to the entrypoints themselves.'); + ..addFlag('help', abbr: 'h', help: 'Print help text.', negatable: false); + + for (var migrator in migrators) { + argParser.addSeparator('${migrator.name} migrator\n' + + '=' * 80 + + '\n${migrator.description}\n${migrator.argParser.usage}'); + argParser.addCommand(migrator.name, migrator.argParser); + } var argResults = argParser.parse(args); - if (argResults['help'] == true || argResults.rest.isEmpty) { - print('Migrates one or more Sass files to the new module system.\n\n' - 'Usage: sass_migrate_to_modules [options] \n\n' - '${argParser.usage}'); - exitCode = 64; + if (argResults['help'] == true || argResults.command == null) { + _printUsage(argParser); return; } - var migrated = migrateFiles(argResults.rest); + Migrator migrator; + + try { + migrator = migrators + .singleWhere((migrator) => migrator.name == argResults.command.name); + } on StateError { + _printUsage(argParser); + return; + } + migrator.argResults = argResults.command; + var migrated = migrator.migrateFiles(argResults.command.rest); if (migrated.isEmpty) { print('Nothing to migrate!'); @@ -56,3 +69,10 @@ void main(List args) { } } } + +void _printUsage(ArgParser argParser) { + print('Runs a migrator on one or more Sass files.\n\n' + 'Usage: sass_migrator [options] \n\n' + '${argParser.usage}'); + exitCode = 64; +} diff --git a/lib/src/migrator.dart b/lib/src/migrator.dart index 18fb9ad2..20742e1f 100644 --- a/lib/src/migrator.dart +++ b/lib/src/migrator.dart @@ -1,362 +1,56 @@ -// Copyright 2018 Google LLC +// Copyright 2019 Google LLC // // Use of this source code is governed by an MIT-style // license that can be found in the LICENSE file or at // https://opensource.org/licenses/MIT. -// The sass package's API is not necessarily stable. It is being imported with -// the Sass team's explicit knowledge and approval. See -// https://github.com/sass/dart-sass/issues/236. -import 'package:sass/src/ast/sass.dart'; -import 'package:sass/src/visitor/recursive_statement.dart'; -import 'package:sass/src/visitor/interface/expression.dart'; - -import 'package:source_span/source_span.dart'; +import 'package:args/args.dart'; import 'package:path/path.dart' as p; -import 'built_in_functions.dart'; -import 'local_scope.dart'; -import 'patch.dart'; -import 'stylesheet_migration.dart'; -import 'utils.dart'; - -/// Runs a migration of multiple [entrypoints] and their dependencies without -/// writing any changes to disk. -/// -/// Each entrypoint migrates all of its dependencies separately from the other -/// entrypoints. Certain stylesheets may be migrated multiple times. If the -/// migrated text of a stylesheet for each run is not identical, this will -/// error. -/// -/// If [directory] is provided, the entrypoints will be interpreted relative to -/// it. Otherwise, they'll be interpreted relative to the current directory. -/// -/// Entrypoints and dependencies that did not require any changes will not be -/// included in the results. -p.PathMap migrateFiles(Iterable entrypoints, - {String directory}) { - var allMigrated = p.PathMap(); - for (var entrypoint in entrypoints) { - var migrated = _Migrator(directory: directory).migrate(entrypoint); - for (var file in migrated.keys) { - if (allMigrated.containsKey(file) && - migrated[file] != allMigrated[file]) { - throw UnsupportedError( - "$file is migrated in more than one way by these entrypoints."); - } - allMigrated[file] = migrated[file]; - } - } - return allMigrated; -} +abstract class Migrator { + /// Name passed at the command line to use this migrator. + String get name; -class _Migrator extends RecursiveStatementVisitor implements ExpressionVisitor { - /// List of all migrations for files touched by this run. - final _migrations = p.PathMap(); + /// Brief description of what this migrator does. + String get description; - /// List of migrations in progress. The last item is the current migration. - final _activeMigrations = []; + /// Parser for the arguments that this migrator takes. + ArgParser get argParser => ArgParser(); - /// Global variables defined at any time during the migrator run. - final _variables = normalizedMap(); + /// Set by the executable based on [argParser] and the provided arguments. + ArgResults argResults; - /// Global mixins defined at any time during the migrator run. - final _mixins = normalizedMap(); - - /// Global functions defined at any time during the migrator run. - final _functions = normalizedMap(); - - /// Directory this migration is run from. - final String _directory; - - _Migrator({String directory}) : _directory = directory ?? p.current; - - /// Local variables, mixins, and functions for migrations in progress. + /// Runs this migrator on [entrypoint], returning a map of migrated contents. /// - /// The migrator will modify this as it traverses stylesheets. When at the - /// top level of a stylesheet, this will be null. - LocalScope _localScope; - - /// Current stylesheet being actively migrated. - StylesheetMigration get _currentMigration => - _activeMigrations.isNotEmpty ? _activeMigrations.last : null; - - /// Runs the migrator on [entrypoint] and its dependencies and returns a map - /// of migrated contents. - p.PathMap migrate(String entrypoint) { - _migrateStylesheet(entrypoint); - var results = p.PathMap(); - for (var migration in _migrations.values) { - results[migration.path] = migration.migratedContents; - } - return results; - } - - /// Migrates the stylesheet at [path] if it hasn't already been migrated and - /// returns the StylesheetMigration instance for it regardless. - StylesheetMigration _migrateStylesheet(String path) { - path = canonicalizePath(p.join( - _currentMigration == null - ? _directory - : p.dirname(_currentMigration.path), - path)); - return _migrations.putIfAbsent(path, () { - var migration = StylesheetMigration(path); - _activeMigrations.add(migration); - visitStylesheet(migration.stylesheet); - _activeMigrations.remove(migration); - return migration; - }); - } - - /// Visits the children of [node] with a local scope. + /// This may also migrate dependencies of this entrypoint, depending on the + /// migrator and its configuration. /// - /// Note: The children of a stylesheet are at the root, so we should not add - /// a local scope. - @override - void visitChildren(ParentStatement node) { - if (node is Stylesheet) { - super.visitChildren(node); - return; - } - _localScope = LocalScope(_localScope); - super.visitChildren(node); - _localScope = _localScope.parent; - } - - /// Adds a namespace to any function call that require it. - void visitFunctionExpression(FunctionExpression node) { - visitInterpolation(node.name); - _patchNamespaceForFunction(node.name.asPlain, (name, namespace) { - _currentMigration.patches.add(Patch(node.name.span, "$namespace.$name")); - }); - visitArgumentInvocation(node.arguments); - - if (node.name.asPlain == "get-function") { - var nameArgument = - node.arguments.named['name'] ?? node.arguments.positional.first; - if (nameArgument is! StringExpression || - (nameArgument as StringExpression).text.asPlain == null) { - print(nameArgument.span.message( - "WARNING - get-function call may require \$module parameter")); - return; - } - var fnName = nameArgument as StringExpression; - _patchNamespaceForFunction(fnName.text.asPlain, (name, namespace) { - var span = fnName.span; - if (fnName.hasQuotes) { - span = span.file.span(span.start.offset + 1, span.end.offset - 1); - } - _currentMigration.patches.add(Patch(span, name)); - var beforeParen = node.span.end.offset - 1; - _currentMigration.patches.add(Patch( - node.span.file.span(beforeParen, beforeParen), - ', \$module: "$namespace"')); - }); - } - } + /// Files that did not require migration, even if touched by the migrator, + /// should not be included map of results. + p.PathMap migrateFile(String entrypoint); - /// Calls [patcher] when the function [name] requires a namespace and adds a - /// new use rule if necessary. + /// Runs this migrator on multiple [entrypoints], returning a merged map of + /// migrated contents. /// - /// [patcher] takes two arguments: the name used to refer to that function - /// when namespaced, and the namespace itself. The name will match the name - /// provided to the outer function except for built-in functions whose name - /// within a module differs from its original name. - void _patchNamespaceForFunction( - String name, void patcher(String name, String namespace)) { - if (name == null) return; - if (_localScope?.isLocalFunction(name) ?? false) return; - - var namespace = _functions.containsKey(name) - ? _currentMigration.namespaceForNode(_functions[name]) - : null; - - if (namespace == null) { - if (!builtInFunctionModules.containsKey(name)) return; - - namespace = builtInFunctionModules[name]; - name = builtInFunctionNameChanges[name] ?? name; - _currentMigration.additionalUseRules.add("sass:$namespace"); - } - if (namespace != null) patcher(name, namespace); - } - - /// Declares the function within the current scope before visiting it. - @override - void visitFunctionRule(FunctionRule node) { - _declareFunction(node); - super.visitFunctionRule(node); - } - - /// Migrates @import to @use after migrating the imported file. - void visitImportRule(ImportRule node) { - if (node.imports.first is StaticImport) { - super.visitImportRule(node); - return; - } - if (node.imports.length > 1) { - throw UnimplementedError( - "Migration of @import rule with multiple imports not supported."); - } - var import = node.imports.first as DynamicImport; - - if (_localScope != null) { - // TODO(jathak): Handle nested imports - return; - } - // TODO(jathak): Confirm that this import appears before other rules - - var importMigration = _migrateStylesheet(import.url); - _currentMigration.namespaces[importMigration.path] = - namespaceForPath(import.url); - - var overrides = []; - for (var variable in importMigration.configurableVariables) { - if (_variables.containsKey(variable)) { - var declaration = _variables[variable]; - if (_currentMigration.namespaceForNode(declaration) == null) { - overrides.add("\$${declaration.name}: ${declaration.expression}"); + /// Each entrypoint is migrated separately. If a stylesheet is migrated more + /// than once, the resulting migrated contents must be the same each time, or + /// this will error. + /// + /// Entrypoints and dependencies that did not require any changes will not be + /// included in the results. + p.PathMap migrateFiles(Iterable entrypoints) { + var allMigrated = p.PathMap(); + for (var entrypoint in entrypoints) { + var migrated = migrateFile(entrypoint); + for (var file in migrated.keys) { + if (allMigrated.containsKey(file) && + migrated[file] != allMigrated[file]) { + throw UnsupportedError( + "$file is migrated in more than one way by these entrypoints."); } - // TODO(jathak): Remove this declaration from the current stylesheet if - // it's not referenced before this point. + allMigrated[file] = migrated[file]; } } - var config = ""; - if (overrides.isNotEmpty) { - config = " with (\n " + overrides.join(',\n ') + "\n)"; - } - _currentMigration.patches - .add(Patch(node.span, '@use ${import.span.text}$config')); + return allMigrated; } - - /// Adds a namespace to any mixin include that requires it. - @override - void visitIncludeRule(IncludeRule node) { - super.visitIncludeRule(node); - if (_localScope?.isLocalMixin(node.name) ?? false) return; - if (!_mixins.containsKey(node.name)) return; - var namespace = _currentMigration.namespaceForNode(_mixins[node.name]); - if (namespace == null) return; - var endName = node.arguments.span.start.offset; - var startName = endName - node.name.length; - var nameSpan = node.span.file.span(startName, endName); - _currentMigration.patches.add(Patch(nameSpan, "$namespace.${node.name}")); - } - - /// Declares the mixin within the current scope before visiting it. - @override - void visitMixinRule(MixinRule node) { - _declareMixin(node); - super.visitMixinRule(node); - } - - @override - void visitUseRule(UseRule node) { - // TODO(jathak): Handle existing @use rules. - } - - /// Adds a namespace to any variable that requires it. - visitVariableExpression(VariableExpression node) { - if (_localScope?.isLocalVariable(node.name) ?? false) { - return; - } - if (!_variables.containsKey(node.name)) return; - var namespace = _currentMigration.namespaceForNode(_variables[node.name]); - if (namespace == null) return; - _currentMigration.patches - .add(Patch(node.span, "\$$namespace.${node.name}")); - } - - /// Declares a variable within the current scope before visiting it. - @override - void visitVariableDeclaration(VariableDeclaration node) { - _declareVariable(node); - super.visitVariableDeclaration(node); - } - - /// Declares a variable within this stylesheet, in the current local scope if - /// it exists, or as a global variable otherwise. - void _declareVariable(VariableDeclaration node) { - if (_localScope == null || node.isGlobal) { - if (node.isGuarded) { - _currentMigration.configurableVariables.add(node.name); - - // Don't override if variable already exists. - _variables.putIfAbsent(node.name, () => node); - } else { - _variables[node.name] = node; - } - } else { - _localScope.variables.add(node.name); - } - } - - /// Declares a mixin within this stylesheet, in the current local scope if - /// it exists, or as a global mixin otherwise. - void _declareMixin(MixinRule node) { - if (_localScope == null) { - _mixins[node.name] = node; - } else { - _localScope.mixins.add(node.name); - } - } - - /// Declares a function within this stylesheet, in the current local scope if - /// it exists, or as a global function otherwise. - void _declareFunction(FunctionRule node) { - if (_localScope == null) { - _functions[node.name] = node; - } else { - _localScope.functions.add(node.name); - } - } - - // Expression Tree Treversal - - @override - visitExpression(Expression expression) => expression.accept(this); - - visitBinaryOperationExpression(BinaryOperationExpression node) { - node.left.accept(this); - node.right.accept(this); - } - - visitIfExpression(IfExpression node) { - visitArgumentInvocation(node.arguments); - } - - visitListExpression(ListExpression node) { - for (var item in node.contents) { - item.accept(this); - } - } - - visitMapExpression(MapExpression node) { - for (var pair in node.pairs) { - pair.item1.accept(this); - pair.item2.accept(this); - } - } - - visitParenthesizedExpression(ParenthesizedExpression node) { - node.expression.accept(this); - } - - visitStringExpression(StringExpression node) { - visitInterpolation(node.text); - } - - visitUnaryOperationExpression(UnaryOperationExpression node) { - node.operand.accept(this); - } - - // No-Op Expression Tree Leaves - - visitBooleanExpression(BooleanExpression node) {} - visitColorExpression(ColorExpression node) {} - visitNullExpression(NullExpression node) {} - visitNumberExpression(NumberExpression node) {} - visitSelectorExpression(SelectorExpression node) {} - visitValueExpression(ValueExpression node) {} } diff --git a/lib/src/migrator_base.dart b/lib/src/migrator_base.dart new file mode 100644 index 00000000..b86cb986 --- /dev/null +++ b/lib/src/migrator_base.dart @@ -0,0 +1,113 @@ +// Copyright 2019 Google LLC +// +// Use of this source code is governed by an MIT-style +// license that can be found in the LICENSE file or at +// https://opensource.org/licenses/MIT. + +import 'dart:io'; + +// The sass package's API is not necessarily stable. It is being imported with +// the Sass team's explicit knowledge and approval. See +// https://github.com/sass/dart-sass/issues/236. +import 'package:sass/src/ast/sass.dart'; +import 'package:sass/src/syntax.dart'; +import 'package:sass/src/visitor/recursive_statement.dart'; +import 'package:sass/src/visitor/interface/expression.dart'; + +import 'package:path/path.dart' as p; + +import 'migrator.dart'; +import 'patch.dart'; +import 'utils.dart'; + +/// This base migrator parses the stylesheet at each provided entrypoint and +/// recursively visits every statement and expression it contains. +/// +/// Migrators based on this should add appropriate patches to [patches] in +/// overridden methods. +/// +/// On its own, this migrator only touches the entrypoints that are directly +/// provided to it; it does not migrate dependencies. +abstract class MigratorBase extends RecursiveStatementVisitor + with Migrator, ExpressionVisitor { + List _patches = []; + + /// The patches to be applied to the stylesheet being migrated. + /// + /// Subclasses that override this should also override [migrateFile]. + List get patches => _patches; + + /// Runs this migrator on [entrypoint]. + /// + /// This will return either a map containing only the migrated contents of + /// [entrypoint], or an empty map if no migration was necessary. + p.PathMap migrateFile(String entrypoint) { + _patches = []; + var path = canonicalizePath(p.join(Directory.current.path, entrypoint)); + var contents = File(path).readAsStringSync(); + var syntax = Syntax.forPath(path); + visitStylesheet(Stylesheet.parse(contents, syntax, url: path)); + + var results = p.PathMap(); + if (_patches.isNotEmpty) { + results[path] = Patch.applyAll(_patches.first.selection.file, _patches); + } + _patches = null; + return results; + } + + // Expression Tree Traversal + + @override + visitExpression(Expression expression) => expression.accept(this); + + visitBinaryOperationExpression(BinaryOperationExpression node) { + node.left.accept(this); + node.right.accept(this); + } + + visitFunctionExpression(FunctionExpression node) { + visitInterpolation(node.name); + visitArgumentInvocation(node.arguments); + } + + visitIfExpression(IfExpression node) { + visitArgumentInvocation(node.arguments); + } + + visitListExpression(ListExpression node) { + for (var item in node.contents) { + item.accept(this); + } + } + + visitMapExpression(MapExpression node) { + for (var pair in node.pairs) { + pair.item1.accept(this); + pair.item2.accept(this); + } + } + + visitParenthesizedExpression(ParenthesizedExpression node) { + node.expression.accept(this); + } + + visitStringExpression(StringExpression node) { + visitInterpolation(node.text); + } + + visitUnaryOperationExpression(UnaryOperationExpression node) { + node.operand.accept(this); + } + + // Expression Leaves + + visitBooleanExpression(BooleanExpression node) {} + visitColorExpression(ColorExpression node) {} + visitNullExpression(NullExpression node) {} + visitNumberExpression(NumberExpression node) {} + visitSelectorExpression(SelectorExpression node) {} + visitValueExpression(ValueExpression node) {} + visitVariableExpression(VariableExpression node) {} + visitUseRule(UseRule node) {} +} diff --git a/lib/src/migrators/module.dart b/lib/src/migrators/module.dart new file mode 100644 index 00000000..9f2fb967 --- /dev/null +++ b/lib/src/migrators/module.dart @@ -0,0 +1,302 @@ +// Copyright 2018 Google LLC +// +// Use of this source code is governed by an MIT-style +// license that can be found in the LICENSE file or at +// https://opensource.org/licenses/MIT. + +import 'dart:io'; + +// The sass package's API is not necessarily stable. It is being imported with +// the Sass team's explicit knowledge and approval. See +// https://github.com/sass/dart-sass/issues/236. +import 'package:sass/src/ast/sass.dart'; + +import 'package:args/args.dart'; +import 'package:path/path.dart' as p; + +import 'package:sass_migrator/src/migrator_base.dart'; +import 'package:sass_migrator/src/patch.dart'; +import 'package:sass_migrator/src/utils.dart'; + +import 'module/built_in_functions.dart'; +import 'module/local_scope.dart'; +import 'module/stylesheet_migration.dart'; + +class ModuleMigrator extends MigratorBase { + final name = "module"; + final description = "Migrates stylesheets to the new module system."; + final argParser = ArgParser() + ..addFlag('migrate-deps', + abbr: 'd', help: 'Migrate dependencies in addition to entrypoints.'); + + bool get _migrateDependencies => argResults['migrate-deps']; + + /// List of all migrations for files touched by this run. + final _migrations = p.PathMap(); + + /// List of migrations in progress. The last item is the current migration. + final _activeMigrations = []; + + /// Global variables defined at any time during the migrator run. + final _variables = normalizedMap(); + + /// Global mixins defined at any time during the migrator run. + final _mixins = normalizedMap(); + + /// Global functions defined at any time during the migrator run. + final _functions = normalizedMap(); + + /// Local variables, mixins, and functions for migrations in progress. + /// + /// The migrator will modify this as it traverses stylesheets. When at the + /// top level of a stylesheet, this will be null. + LocalScope _localScope; + + /// Current stylesheet being actively migrated. + StylesheetMigration get _currentMigration => + _activeMigrations.isNotEmpty ? _activeMigrations.last : null; + + /// The patches to be aplied to the stylesheet being actively migrated. + List get patches => _currentMigration.patches; + + /// Runs the module migrator on [entrypoint] and its dependencies and returns + /// a map of migrated contents. + /// + /// If [_migrateDependencies] is false, the migrator will still be run on + /// dependencies, but they will be excluded from the resulting map. + @override + p.PathMap migrateFile(String entrypoint) { + var migration = _migrateStylesheet(entrypoint, Directory.current.path); + var results = p.PathMap(); + addMigration(StylesheetMigration migration) { + var migrated = migration.migratedContents; + if (migrated != migration.contents) { + results[migration.path] = migrated; + } + } + + if (_migrateDependencies) { + _migrations.values.forEach(addMigration); + } else { + addMigration(migration); + } + return results; + } + + /// Migrates the stylesheet at [path] if it hasn't already been migrated and + /// returns the StylesheetMigration instance for it regardless. + StylesheetMigration _migrateStylesheet(String path, String directory) { + path = canonicalizePath(p.join(directory, path)); + return _migrations.putIfAbsent(path, () { + var migration = StylesheetMigration(path); + _activeMigrations.add(migration); + visitStylesheet(migration.stylesheet); + _activeMigrations.remove(migration); + return migration; + }); + } + + /// Visits the children of [node] with a local scope. + /// + /// Note: The children of a stylesheet are at the root, so we should not add + /// a local scope. + @override + void visitChildren(ParentStatement node) { + if (node is Stylesheet) { + super.visitChildren(node); + return; + } + _localScope = LocalScope(_localScope); + super.visitChildren(node); + _localScope = _localScope.parent; + } + + /// Adds a namespace to any function call that require it. + void visitFunctionExpression(FunctionExpression node) { + visitInterpolation(node.name); + _patchNamespaceForFunction(node.name.asPlain, (name, namespace) { + _currentMigration.patches.add(Patch(node.name.span, "$namespace.$name")); + }); + visitArgumentInvocation(node.arguments); + + if (node.name.asPlain == "get-function") { + var nameArgument = + node.arguments.named['name'] ?? node.arguments.positional.first; + if (nameArgument is! StringExpression || + (nameArgument as StringExpression).text.asPlain == null) { + warn("get-function call may require \$module parameter", + nameArgument.span); + return; + } + var fnName = nameArgument as StringExpression; + _patchNamespaceForFunction(fnName.text.asPlain, (name, namespace) { + var span = fnName.span; + if (fnName.hasQuotes) { + span = span.file.span(span.start.offset + 1, span.end.offset - 1); + } + _currentMigration.patches.add(Patch(span, name)); + var beforeParen = node.span.end.offset - 1; + _currentMigration.patches.add(Patch( + node.span.file.span(beforeParen, beforeParen), + ', \$module: "$namespace"')); + }); + } + } + + /// Calls [patcher] when the function [name] requires a namespace and adds a + /// new use rule if necessary. + /// + /// [patcher] takes two arguments: the name used to refer to that function + /// when namespaced, and the namespace itself. The name will match the name + /// provided to the outer function except for built-in functions whose name + /// within a module differs from its original name. + void _patchNamespaceForFunction( + String name, void patcher(String name, String namespace)) { + if (name == null) return; + if (_localScope?.isLocalFunction(name) ?? false) return; + + var namespace = _functions.containsKey(name) + ? _currentMigration.namespaceForNode(_functions[name]) + : null; + + if (namespace == null) { + if (!builtInFunctionModules.containsKey(name)) return; + + namespace = builtInFunctionModules[name]; + name = builtInFunctionNameChanges[name] ?? name; + _currentMigration.additionalUseRules.add("sass:$namespace"); + } + if (namespace != null) patcher(name, namespace); + } + + /// Declares the function within the current scope before visiting it. + @override + void visitFunctionRule(FunctionRule node) { + _declareFunction(node); + super.visitFunctionRule(node); + } + + /// Migrates @import to @use after migrating the imported file. + void visitImportRule(ImportRule node) { + if (node.imports.first is StaticImport) { + super.visitImportRule(node); + return; + } + if (node.imports.length > 1) { + throw UnimplementedError( + "Migration of @import rule with multiple imports not supported."); + } + var import = node.imports.first as DynamicImport; + + if (_localScope != null) { + // TODO(jathak): Handle nested imports + return; + } + // TODO(jathak): Confirm that this import appears before other rules + + var importMigration = + _migrateStylesheet(import.url, p.dirname(_currentMigration.path)); + _currentMigration.namespaces[importMigration.path] = + namespaceForPath(import.url); + + var overrides = []; + for (var variable in importMigration.configurableVariables) { + if (_variables.containsKey(variable)) { + var declaration = _variables[variable]; + if (_currentMigration.namespaceForNode(declaration) == null) { + overrides.add("\$${declaration.name}: ${declaration.expression}"); + } + // TODO(jathak): Remove this declaration from the current stylesheet if + // it's not referenced before this point. + } + } + var config = ""; + if (overrides.isNotEmpty) { + config = " with (\n " + overrides.join(',\n ') + "\n)"; + } + _currentMigration.patches + .add(Patch(node.span, '@use ${import.span.text}$config')); + } + + /// Adds a namespace to any mixin include that requires it. + @override + void visitIncludeRule(IncludeRule node) { + super.visitIncludeRule(node); + if (_localScope?.isLocalMixin(node.name) ?? false) return; + if (!_mixins.containsKey(node.name)) return; + var namespace = _currentMigration.namespaceForNode(_mixins[node.name]); + if (namespace == null) return; + var endName = node.arguments.span.start.offset; + var startName = endName - node.name.length; + var nameSpan = node.span.file.span(startName, endName); + _currentMigration.patches.add(Patch(nameSpan, "$namespace.${node.name}")); + } + + /// Declares the mixin within the current scope before visiting it. + @override + void visitMixinRule(MixinRule node) { + _declareMixin(node); + super.visitMixinRule(node); + } + + @override + void visitUseRule(UseRule node) { + // TODO(jathak): Handle existing @use rules. + } + + /// Adds a namespace to any variable that requires it. + visitVariableExpression(VariableExpression node) { + if (_localScope?.isLocalVariable(node.name) ?? false) { + return; + } + if (!_variables.containsKey(node.name)) return; + var namespace = _currentMigration.namespaceForNode(_variables[node.name]); + if (namespace == null) return; + _currentMigration.patches + .add(Patch(node.span, "\$$namespace.${node.name}")); + } + + /// Declares a variable within the current scope before visiting it. + @override + void visitVariableDeclaration(VariableDeclaration node) { + _declareVariable(node); + super.visitVariableDeclaration(node); + } + + /// Declares a variable within this stylesheet, in the current local scope if + /// it exists, or as a global variable otherwise. + void _declareVariable(VariableDeclaration node) { + if (_localScope == null || node.isGlobal) { + if (node.isGuarded) { + _currentMigration.configurableVariables.add(node.name); + + // Don't override if variable already exists. + _variables.putIfAbsent(node.name, () => node); + } else { + _variables[node.name] = node; + } + } else { + _localScope.variables.add(node.name); + } + } + + /// Declares a mixin within this stylesheet, in the current local scope if + /// it exists, or as a global mixin otherwise. + void _declareMixin(MixinRule node) { + if (_localScope == null) { + _mixins[node.name] = node; + } else { + _localScope.mixins.add(node.name); + } + } + + /// Declares a function within this stylesheet, in the current local scope if + /// it exists, or as a global function otherwise. + void _declareFunction(FunctionRule node) { + if (_localScope == null) { + _functions[node.name] = node; + } else { + _localScope.functions.add(node.name); + } + } +} diff --git a/lib/src/migrators/module/built_in_functions.dart b/lib/src/migrators/module/built_in_functions.dart new file mode 100644 index 00000000..72f9677e --- /dev/null +++ b/lib/src/migrators/module/built_in_functions.dart @@ -0,0 +1,102 @@ +/// Mapping from existing built-in function name to the module it's now part of. +const builtInFunctionModules = { + "red": "color", + "blue": "color", + "green": "color", + "mix": "color", + "hue": "color", + "saturation": "color", + "lightness": "color", + "adjust-hue": "color", + "lighten": "color", + "darken": "color", + "saturate": "color", + "desaturate": "color", + "grayscale": "color", + "complement": "color", + "invert": "color", + "alpha": "color", + "opacify": "color", + "transparentize": "color", + "adjust-color": "color", + "scale-color": "color", + "change-color": "color", + "ie-hex-str": "color", + "map-get": "map", + "map-merge": "map", + "map-remove": "map", + "map-keys": "map", + "map-values": "map", + "map-has-key": "map", + "keywords": "map", + "selector-nest": "selector", + "selector-append": "selector", + "selector-replace": "selector", + "selector-unify": "selector", + "is-superselector": "selector", + "simple-selectors": "selector", + "selector-parse": "selector", + "percentage": "math", + "round": "math", + "ceil": "math", + "floor": "math", + "abs": "math", + "min": "math", + "max": "math", + "random": "math", + "unit": "math", + "unitless": "math", + "comparable": "math", + "length": "list", + "nth": "list", + "set-nth": "list", + "join": "list", + "append": "list", + "zip": "list", + "index": "list", + "list-separator": "list", + "feature-exists": "meta", + "variable-exists": "meta", + "global-variable-exists": "meta", + "function-exists": "meta", + "mixin-exists": "meta", + "inspect": "meta", + "get-function": "meta", + "type-of": "meta", + "call": "meta", + "content-exists": "meta", + "unquote": "string", + "quote": "string", + "str-length": "string", + "str-insert": "string", + "str-index": "string", + "str-slice": "string", + "to-upper-case": "string", + "to-lower-case": "string", + "unique-id": "string" +}; + +/// Mapping from old function name to new name, excluding namespace. +const builtInFunctionNameChanges = { + "adjust-color": "adjust", + "scale-color": "scale", + "change-color": "change", + "map-get": "get", + "map-merge": "merge", + "map-remove": "remove", + "map-keys": "keys", + "map-value": "values", + "map-has-key": "has-key", + "selector-nest": "nest", + "selector-append": "append", + "selector-replace": "replace", + "selector-unify": "unify", + "selector-parse": "parse", + "unitless": "is-unitless", + "comparable": "compatible", + "list-separator": "separator", + "str-length": "length", + "str-insert": "insert", + "str-index": "index", + "str-slice": "slice" +}; diff --git a/lib/src/migrators/module/local_scope.dart b/lib/src/migrators/module/local_scope.dart new file mode 100644 index 00000000..7307532b --- /dev/null +++ b/lib/src/migrators/module/local_scope.dart @@ -0,0 +1,38 @@ +// Copyright 2019 Google LLC +// +// Use of this source code is governed by an MIT-style +// license that can be found in the LICENSE file or at +// https://opensource.org/licenses/MIT. + +import 'package:sass_migrator/src/utils.dart'; + +/// Keeps track of the scope of any members declared at the current level of +/// the stylesheet. +class LocalScope { + /// The parent of this scope, or null if this scope is only nested one level + /// from the root of the file. + final LocalScope parent; + + /// Variables defined in this scope. + final variables = normalizedSet(); + + /// Mixins defined in this scope. + final mixins = normalizedSet(); + + /// Functions defined in this scope. + final functions = normalizedSet(); + + LocalScope(this.parent); + + /// Returns whether a variable [name] exists somewhere within this scope. + bool isLocalVariable(String name) => + variables.contains(name) || (parent?.isLocalVariable(name) ?? false); + + /// Returns whether a mixin [name] exists somewhere within this scope. + bool isLocalMixin(String name) => + variables.contains(name) || (parent?.isLocalMixin(name) ?? false); + + /// Returns whether a function [name] exists somewhere within this scope. + bool isLocalFunction(String name) => + variables.contains(name) || (parent?.isLocalFunction(name) ?? false); +} diff --git a/lib/src/migrators/module/stylesheet_migration.dart b/lib/src/migrators/module/stylesheet_migration.dart new file mode 100644 index 00000000..68c7ba4d --- /dev/null +++ b/lib/src/migrators/module/stylesheet_migration.dart @@ -0,0 +1,85 @@ +// Copyright 2019 Google LLC +// +// Use of this source code is governed by an MIT-style +// license that can be found in the LICENSE file or at +// https://opensource.org/licenses/MIT. + +import 'dart:io'; + +import 'package:path/path.dart' as p; + +// The sass package's API is not necessarily stable. It is being imported with +// the Sass team's explicit knowledge and approval. See +// https://github.com/sass/dart-sass/issues/236. +import 'package:sass/src/ast/sass.dart'; +import 'package:sass/src/syntax.dart'; + +import 'package:sass_migrator/src/patch.dart'; +import 'package:sass_migrator/src/utils.dart'; + +/// Represents an in-progress migration for a stylesheet. +class StylesheetMigration { + /// The stylesheet this migration is for. + final Stylesheet stylesheet; + + /// The canonical path of this stylesheet. + final String path; + + /// The original contents of this stylesheet, prior to migration. + final String contents; + + /// The syntax used in this stylesheet. + final Syntax syntax; + + /// Namespaces of modules used in this stylesheet. + final namespaces = p.PathMap(); + + /// Set of additional use rules necessary for referencing members of + /// implicit dependencies / built-in modules. + /// + /// This set contains the path provided in the use rule, not the canonical + /// path (e.g. "a" rather than "dir/a.scss"). + final additionalUseRules = Set(); + + /// List of patches to be applied to this file. + final patches = []; + + /// Global variables declared with !default that could be configured. + final configurableVariables = normalizedSet(); + + StylesheetMigration._(this.stylesheet, this.path, this.contents, this.syntax); + + /// Creates a new migration for the stylesheet at [path]. + factory StylesheetMigration(String path) { + var contents = File(path).readAsStringSync(); + var syntax = Syntax.forPath(path); + var stylesheet = Stylesheet.parse(contents, syntax, url: path); + return StylesheetMigration._(stylesheet, path, contents, syntax); + } + + /// Returns the migrated contents of this file, based on [additionalUseRules] + /// and [patches]. + String get migratedContents { + var semicolon = syntax == Syntax.sass ? "" : ";"; + var uses = additionalUseRules.map((use) => '@use "$use"$semicolon\n'); + var contents = Patch.applyAll(stylesheet.span.file, patches); + return uses.join("") + contents; + } + + /// Finds the namespace for the stylesheet containing [node], adding a new use + /// rule if necessary. + String namespaceForNode(SassNode node) { + var nodePath = p.fromUri(node.span.sourceUrl); + if (p.equals(nodePath, path)) return null; + if (!namespaces.containsKey(nodePath)) { + /// Add new use rule for indirect dependency + var relativePath = p.relative(nodePath, from: p.dirname(path)); + var basename = p.basenameWithoutExtension(relativePath); + if (basename.startsWith('_')) basename = basename.substring(1); + var simplePath = p.relative(p.join(p.dirname(relativePath), basename)); + additionalUseRules.add(simplePath); + namespaces[nodePath] = namespaceForPath(simplePath); + } + return namespaces[nodePath]; + } +} diff --git a/lib/src/utils.dart b/lib/src/utils.dart index 55b78f88..b29f77e7 100644 --- a/lib/src/utils.dart +++ b/lib/src/utils.dart @@ -5,13 +5,17 @@ // https://opensource.org/licenses/MIT. import 'package:path/path.dart' as p; +import 'package:source_span/source_span.dart'; // The sass package's API is not necessarily stable. It is being imported with // the Sass team's explicit knowledge and approval. See // https://github.com/sass/dart-sass/issues/236. +import 'package:sass/src/ast/node.dart'; import 'package:sass/src/importer/utils.dart' show resolveImportPath; export 'package:sass/src/utils.dart' show normalizedMap, normalizedSet; +import 'patch.dart'; + /// Returns the canonical version of [path]. String canonicalizePath(String path) { return p.canonicalize(resolveImportPath(path)); @@ -22,3 +26,20 @@ String namespaceForPath(String path) { // TODO(jathak): Confirm that this is a valid Sass identifier return path.split('/').last.split('.').first; } + +/// Creates a patch that adds [text] immediately before [node]. +Patch patchBefore(AstNode node, String text) { + var start = node.span.start; + return Patch(start.file.span(start.offset, start.offset), text); +} + +/// Creates a patch that adds [text] immediately after [node]. +Patch patchAfter(AstNode node, String text) { + var end = node.span.end; + return Patch(end.file.span(end.offset, end.offset), text); +} + +/// Emits a warning with [message] and [context]; +void warn(String message, FileSpan context) { + print(context.message("WARNING - $message")); +} diff --git a/pubspec.yaml b/pubspec.yaml index 4034355b..7c95e5d6 100644 --- a/pubspec.yaml +++ b/pubspec.yaml @@ -1,13 +1,13 @@ -name: sass_module_migrator +name: sass_migrator version: 0.0.1-dev -description: A tool for migrating Sass files to the new module system +description: A tool for running migrations on Sass files environment: sdk: '>=2.1.0 <3.0.0' dependencies: args: "^1.4.0" - sass: "^1.17.2" + sass: "^1.17.3" source_span: "^1.4.0" path: "^1.6.0" diff --git a/test/migrations/README.md b/test/migrations/README.md index 1a9e84d5..fc8ba508 100644 --- a/test/migrations/README.md +++ b/test/migrations/README.md @@ -1,6 +1,8 @@ # Migration Tests -Each set of source files used for a test of the migrator is represented a single +For each migrator, this folder contains a subdirectory with tests. + +Each set of source files used for a test of a migrator is represented a single [HRX archive](https://github.com/google/hrx). > Note: The test script does not currently use a proper HRX parser, so `<==>` is diff --git a/test/migrations/globally_shadowed_variable.hrx b/test/migrations/module/globally_shadowed_variable.hrx similarity index 87% rename from test/migrations/globally_shadowed_variable.hrx rename to test/migrations/module/globally_shadowed_variable.hrx index f0dec359..ee9987dc 100644 --- a/test/migrations/globally_shadowed_variable.hrx +++ b/test/migrations/module/globally_shadowed_variable.hrx @@ -1,3 +1,6 @@ +<==> arguments +--migrate-deps + <==> input/entrypoint.scss @import "library"; $variable : red; diff --git a/test/migrations/module_configuration.hrx b/test/migrations/module/module_configuration.hrx similarity index 88% rename from test/migrations/module_configuration.hrx rename to test/migrations/module/module_configuration.hrx index 67393975..c7fe9ac8 100644 --- a/test/migrations/module_configuration.hrx +++ b/test/migrations/module/module_configuration.hrx @@ -1,3 +1,6 @@ +<==> arguments +--migrate-deps + <==> input/entrypoint.scss $config: red; $no-config: blue; diff --git a/test/migrations/namespace_builtin_functions.hrx b/test/migrations/module/namespace_builtin_functions.hrx similarity index 100% rename from test/migrations/namespace_builtin_functions.hrx rename to test/migrations/module/namespace_builtin_functions.hrx diff --git a/test/migrations/namespace_function.hrx b/test/migrations/module/namespace_function.hrx similarity index 87% rename from test/migrations/namespace_function.hrx rename to test/migrations/module/namespace_function.hrx index a22bba52..3242a71c 100644 --- a/test/migrations/namespace_function.hrx +++ b/test/migrations/module/namespace_function.hrx @@ -1,3 +1,6 @@ +<==> arguments +--migrate-deps + <==> input/entrypoint.scss @import "library"; a { diff --git a/test/migrations/namespace_get_function.hrx b/test/migrations/module/namespace_get_function.hrx similarity index 97% rename from test/migrations/namespace_get_function.hrx rename to test/migrations/module/namespace_get_function.hrx index 56854188..2f9071ec 100644 --- a/test/migrations/namespace_get_function.hrx +++ b/test/migrations/module/namespace_get_function.hrx @@ -1,3 +1,6 @@ +<==> arguments +--migrate-deps + <==> input/entrypoint.scss @import "library"; @import "true"; diff --git a/test/migrations/namespace_indirect_reference.hrx b/test/migrations/module/namespace_indirect_reference.hrx similarity index 90% rename from test/migrations/namespace_indirect_reference.hrx rename to test/migrations/module/namespace_indirect_reference.hrx index dc092b0c..2013873e 100644 --- a/test/migrations/namespace_indirect_reference.hrx +++ b/test/migrations/module/namespace_indirect_reference.hrx @@ -1,3 +1,6 @@ +<==> arguments +--migrate-deps + <==> input/entrypoint.scss @import "direct"; a { diff --git a/test/migrations/namespace_mixin.hrx b/test/migrations/module/namespace_mixin.hrx similarity index 87% rename from test/migrations/namespace_mixin.hrx rename to test/migrations/module/namespace_mixin.hrx index 2e430623..719887c0 100644 --- a/test/migrations/namespace_mixin.hrx +++ b/test/migrations/module/namespace_mixin.hrx @@ -1,3 +1,6 @@ +<==> arguments +--migrate-deps + <==> input/entrypoint.scss @import "library"; a { diff --git a/test/migrations/namespace_variable.hrx b/test/migrations/module/namespace_variable.hrx similarity index 86% rename from test/migrations/namespace_variable.hrx rename to test/migrations/module/namespace_variable.hrx index ad165a97..e09fd486 100644 --- a/test/migrations/namespace_variable.hrx +++ b/test/migrations/module/namespace_variable.hrx @@ -1,3 +1,6 @@ +<==> arguments +--migrate-deps + <==> input/entrypoint.scss @import "library"; a { diff --git a/test/migrations/subdirectories.hrx b/test/migrations/module/subdirectories.hrx similarity index 92% rename from test/migrations/subdirectories.hrx rename to test/migrations/module/subdirectories.hrx index bd371af0..3d4e311c 100644 --- a/test/migrations/subdirectories.hrx +++ b/test/migrations/module/subdirectories.hrx @@ -1,3 +1,6 @@ +<==> arguments +--migrate-deps + <==> input/entrypoint.scss @import "folder/inner1"; $result: $a; diff --git a/test/migrator_test.dart b/test/migrator_test.dart index 888ab829..751ff9ce 100644 --- a/test/migrator_test.dart +++ b/test/migrator_test.dart @@ -6,7 +6,8 @@ import 'dart:io'; -import 'package:sass_module_migrator/src/migrator.dart'; +import 'package:sass_migrator/src/migrator.dart'; +import 'package:sass_migrator/src/migrators/module.dart'; import 'package:path/path.dart' as p; import 'package:term_glyph/term_glyph.dart' as glyph; @@ -16,26 +17,34 @@ import 'package:test_descriptor/test_descriptor.dart' as d; /// Runs all migration tests. See migrations/README.md for details. void main() { glyph.ascii = true; - var migrationTests = Directory("test/migrations"); - for (var file in migrationTests.listSync().whereType()) { - if (file.path.endsWith(".hrx")) { - test(p.basenameWithoutExtension(file.path), () => testHrx(file)); + testMigrator(ModuleMigrator()); +} + +void testMigrator(Migrator migrator) { + var migrationTests = Directory("test/migrations/${migrator.name}"); + group(migrator.name, () { + for (var file in migrationTests.listSync().whereType()) { + if (file.path.endsWith(".hrx")) { + test(p.basenameWithoutExtension(file.path), + () => testHrx(file, migrator)); + } } - } + }); } /// Run the migration test in [hrxFile]. See migrations/README.md for details. -testHrx(File hrxFile) async { +testHrx(File hrxFile, Migrator migrator) async { var files = HrxTestFiles(hrxFile.readAsStringSync()); await files.unpack(); var entrypoints = files.input.keys.where((path) => path.startsWith("entrypoint")); p.PathMap migrated; - var migration = () { - migrated = migrateFiles(entrypoints, directory: d.sandbox); - }; - expect(migration, - prints(files.expectedLog?.replaceAll("\$TEST_DIR", d.sandbox) ?? "")); + migrator.argResults = migrator.argParser.parse(files.arguments); + expect(() { + IOOverrides.runZoned(() { + migrated = migrator.migrateFiles(entrypoints); + }, getCurrentDirectory: () => Directory(d.sandbox)); + }, prints(files.expectedLog?.replaceAll("\$TEST_DIR", d.sandbox) ?? "")); for (var file in files.input.keys) { expect(migrated[p.join(d.sandbox, file)], equals(files.output[file]), reason: 'Incorrect migration of $file.'); @@ -45,6 +54,7 @@ testHrx(File hrxFile) async { class HrxTestFiles { Map input = {}; Map output = {}; + List arguments = []; String expectedLog; HrxTestFiles(String hrxText) { @@ -72,6 +82,8 @@ class HrxTestFiles { output[filename.substring(7)] = contents; } else if (filename == "log.txt") { expectedLog = contents; + } else if (filename == "arguments") { + arguments = contents.trim().split(" "); } } diff --git a/test/patch_test.dart b/test/patch_test.dart index c64b1e45..5c1aee9f 100644 --- a/test/patch_test.dart +++ b/test/patch_test.dart @@ -4,7 +4,7 @@ // license that can be found in the LICENSE file or at // https://opensource.org/licenses/MIT. -import 'package:sass_module_migrator/src/patch.dart'; +import 'package:sass_migrator/src/patch.dart'; import 'package:source_span/source_span.dart'; import 'package:test/test.dart'; From ac3b394aa1cba64d77082870f0730fe739b1c2d0 Mon Sep 17 00:00:00 2001 From: Jennifer Thakar Date: Thu, 4 Apr 2019 16:41:01 -0700 Subject: [PATCH 2/7] Generalizes more of the migrator --migrate-deps is now a global flag, so additional migrators should not need to re-implement dependency migration unless they need additional special logic like the module migrator needs. The generic migration classes are now split into Migrator and MigrationVisitor. Each Migrator is a Command and should contain state for the entire migration run. Most migrators will also want to create a private subclass of MigrationVisitor that they call from Migrator.migrateFile. A new instance of the MigrationVisitor will be created for each stylesheet being migrated, so it contains per-stylesheet state, similar to what StylesheetMigration used to be for the module migrator. --- bin/sass_migrator.dart | 75 +----- lib/runner.dart | 59 +++++ lib/src/built_in_functions.dart | 102 -------- lib/src/local_scope.dart | 38 --- lib/src/migration_visitor.dart | 101 ++++++++ lib/src/migrator.dart | 54 +++-- lib/src/migrator_base.dart | 113 --------- lib/src/migrators/module.dart | 223 +++++++++--------- .../module/stylesheet_migration.dart | 85 ------- lib/src/stylesheet_migration.dart | 86 ------- lib/src/utils.dart | 15 ++ pubspec.yaml | 6 +- test/migrator_test.dart | 27 +-- 13 files changed, 343 insertions(+), 641 deletions(-) create mode 100644 lib/runner.dart delete mode 100644 lib/src/built_in_functions.dart delete mode 100644 lib/src/local_scope.dart create mode 100644 lib/src/migration_visitor.dart delete mode 100644 lib/src/migrator_base.dart delete mode 100644 lib/src/migrators/module/stylesheet_migration.dart delete mode 100644 lib/src/stylesheet_migration.dart diff --git a/bin/sass_migrator.dart b/bin/sass_migrator.dart index 882629bf..9e32cde0 100644 --- a/bin/sass_migrator.dart +++ b/bin/sass_migrator.dart @@ -1,78 +1,9 @@ -// Copyright 2018 Google LLC +// Copyright 2019 Google LLC // // Use of this source code is governed by an MIT-style // license that can be found in the LICENSE file or at // https://opensource.org/licenses/MIT. -import 'dart:io'; +import 'package:sass_migrator/runner.dart'; -import 'package:args/args.dart'; - -import 'package:sass_migrator/src/migrator.dart'; -import 'package:sass_migrator/src/migrators/module.dart'; - -final migrators = [ModuleMigrator()]; - -void main(List args) { - var argParser = ArgParser(usageLineLength: 80) - ..addFlag('dry-run', - abbr: 'n', - help: 'Show which files would be migrated but make no changes.') - ..addFlag('verbose', - abbr: 'v', - help: 'Print text of migrated files when running with --dry-run.') - ..addFlag('help', abbr: 'h', help: 'Print help text.', negatable: false); - - for (var migrator in migrators) { - argParser.addSeparator('${migrator.name} migrator\n' + - '=' * 80 + - '\n${migrator.description}\n${migrator.argParser.usage}'); - argParser.addCommand(migrator.name, migrator.argParser); - } - var argResults = argParser.parse(args); - - if (argResults['help'] == true || argResults.command == null) { - _printUsage(argParser); - return; - } - - Migrator migrator; - - try { - migrator = migrators - .singleWhere((migrator) => migrator.name == argResults.command.name); - } on StateError { - _printUsage(argParser); - return; - } - migrator.argResults = argResults.command; - var migrated = migrator.migrateFiles(argResults.command.rest); - - if (migrated.isEmpty) { - print('Nothing to migrate!'); - return; - } - - if (argResults['dry-run']) { - print('Dry run. Logging migrated files instead of overwriting...\n'); - for (var path in migrated.keys) { - print('$path'); - if (argResults['verbose']) { - print('=' * 80); - print(migrated[path]); - } - } - } else { - for (var path in migrated.keys) { - if (argResults['verbose']) print("Overwriting $path..."); - File(path).writeAsStringSync(migrated[path]); - } - } -} - -void _printUsage(ArgParser argParser) { - print('Runs a migrator on one or more Sass files.\n\n' - 'Usage: sass_migrator [options] \n\n' - '${argParser.usage}'); - exitCode = 64; -} +main(List args) => MigratorRunner().execute(args); diff --git a/lib/runner.dart b/lib/runner.dart new file mode 100644 index 00000000..9bf8b912 --- /dev/null +++ b/lib/runner.dart @@ -0,0 +1,59 @@ +// Copyright 2019 Google LLC +// +// Use of this source code is governed by an MIT-style +// license that can be found in the LICENSE file or at +// https://opensource.org/licenses/MIT. + +import 'dart:io'; + +import 'package:args/command_runner.dart'; +import 'package:path/path.dart' as p; + +import 'src/migrators/module.dart'; + +/// A command runner that runs a migrator based on provided arguments. +class MigratorRunner extends CommandRunner> { + final invocation = "sass_migrator [options] "; + + MigratorRunner() + : super("sass_migrator", "Migrates stylesheets to new Sass versions.") { + argParser.addFlag('migrate-deps', + abbr: 'd', help: 'Migrate dependencies in addition to entrypoints.'); + argParser.addFlag('dry-run', + abbr: 'n', + help: 'Show which files would be migrated but make no changes.'); + argParser.addFlag('verbose', + abbr: 'v', + help: 'Print text of migrated files when running with --dry-run.'); + addCommand(ModuleMigrator()); + } + + /// Runs a migrator and then writes the migrated files to disk unless + /// --dry-run is passed. + Future execute(Iterable args) async { + var argResults = parse(args); + var migrated = await runCommand(argResults); + if (migrated == null) return; + + if (migrated.isEmpty) { + print('Nothing to migrate!'); + return; + } + + if (argResults['dry-run']) { + print('Dry run. Logging migrated files instead of overwriting...\n'); + for (var path in migrated.keys) { + print('$path'); + if (argResults['verbose']) { + print('=' * 80); + print(migrated[path]); + } + } + } else { + for (var path in migrated.keys) { + if (argResults['verbose']) print("Overwriting $path..."); + File(path).writeAsStringSync(migrated[path]); + } + } + } +} diff --git a/lib/src/built_in_functions.dart b/lib/src/built_in_functions.dart deleted file mode 100644 index 72f9677e..00000000 --- a/lib/src/built_in_functions.dart +++ /dev/null @@ -1,102 +0,0 @@ -/// Mapping from existing built-in function name to the module it's now part of. -const builtInFunctionModules = { - "red": "color", - "blue": "color", - "green": "color", - "mix": "color", - "hue": "color", - "saturation": "color", - "lightness": "color", - "adjust-hue": "color", - "lighten": "color", - "darken": "color", - "saturate": "color", - "desaturate": "color", - "grayscale": "color", - "complement": "color", - "invert": "color", - "alpha": "color", - "opacify": "color", - "transparentize": "color", - "adjust-color": "color", - "scale-color": "color", - "change-color": "color", - "ie-hex-str": "color", - "map-get": "map", - "map-merge": "map", - "map-remove": "map", - "map-keys": "map", - "map-values": "map", - "map-has-key": "map", - "keywords": "map", - "selector-nest": "selector", - "selector-append": "selector", - "selector-replace": "selector", - "selector-unify": "selector", - "is-superselector": "selector", - "simple-selectors": "selector", - "selector-parse": "selector", - "percentage": "math", - "round": "math", - "ceil": "math", - "floor": "math", - "abs": "math", - "min": "math", - "max": "math", - "random": "math", - "unit": "math", - "unitless": "math", - "comparable": "math", - "length": "list", - "nth": "list", - "set-nth": "list", - "join": "list", - "append": "list", - "zip": "list", - "index": "list", - "list-separator": "list", - "feature-exists": "meta", - "variable-exists": "meta", - "global-variable-exists": "meta", - "function-exists": "meta", - "mixin-exists": "meta", - "inspect": "meta", - "get-function": "meta", - "type-of": "meta", - "call": "meta", - "content-exists": "meta", - "unquote": "string", - "quote": "string", - "str-length": "string", - "str-insert": "string", - "str-index": "string", - "str-slice": "string", - "to-upper-case": "string", - "to-lower-case": "string", - "unique-id": "string" -}; - -/// Mapping from old function name to new name, excluding namespace. -const builtInFunctionNameChanges = { - "adjust-color": "adjust", - "scale-color": "scale", - "change-color": "change", - "map-get": "get", - "map-merge": "merge", - "map-remove": "remove", - "map-keys": "keys", - "map-value": "values", - "map-has-key": "has-key", - "selector-nest": "nest", - "selector-append": "append", - "selector-replace": "replace", - "selector-unify": "unify", - "selector-parse": "parse", - "unitless": "is-unitless", - "comparable": "compatible", - "list-separator": "separator", - "str-length": "length", - "str-insert": "insert", - "str-index": "index", - "str-slice": "slice" -}; diff --git a/lib/src/local_scope.dart b/lib/src/local_scope.dart deleted file mode 100644 index febd5c1f..00000000 --- a/lib/src/local_scope.dart +++ /dev/null @@ -1,38 +0,0 @@ -// Copyright 2019 Google LLC -// -// Use of this source code is governed by an MIT-style -// license that can be found in the LICENSE file or at -// https://opensource.org/licenses/MIT. - -import 'utils.dart'; - -/// Keeps track of the scope of any members declared at the current level of -/// the stylesheet. -class LocalScope { - /// The parent of this scope, or null if this scope is only nested one level - /// from the root of the file. - final LocalScope parent; - - /// Variables defined in this scope. - final variables = normalizedSet(); - - /// Mixins defined in this scope. - final mixins = normalizedSet(); - - /// Functions defined in this scope. - final functions = normalizedSet(); - - LocalScope(this.parent); - - /// Returns whether a variable [name] exists somewhere within this scope. - bool isLocalVariable(String name) => - variables.contains(name) || (parent?.isLocalVariable(name) ?? false); - - /// Returns whether a mixin [name] exists somewhere within this scope. - bool isLocalMixin(String name) => - variables.contains(name) || (parent?.isLocalMixin(name) ?? false); - - /// Returns whether a function [name] exists somewhere within this scope. - bool isLocalFunction(String name) => - variables.contains(name) || (parent?.isLocalFunction(name) ?? false); -} diff --git a/lib/src/migration_visitor.dart b/lib/src/migration_visitor.dart new file mode 100644 index 00000000..10d823e4 --- /dev/null +++ b/lib/src/migration_visitor.dart @@ -0,0 +1,101 @@ +// Copyright 2019 Google LLC +// +// Use of this source code is governed by an MIT-style +// license that can be found in the LICENSE file or at +// https://opensource.org/licenses/MIT. + +import 'dart:io'; + +// The sass package's API is not necessarily stable. It is being imported with +// the Sass team's explicit knowledge and approval. See +// https://github.com/sass/dart-sass/issues/236. +import 'package:sass/src/ast/sass.dart'; +import 'package:sass/src/syntax.dart'; +import 'package:sass/src/visitor/recursive_ast.dart'; + +import 'package:path/path.dart' as p; + +import 'migrator.dart'; +import 'patch.dart'; +import 'utils.dart'; + +/// A visitor that migrates a stylesheet. +/// +/// When [run] is called, this visitor traverses a stylesheet's AST, allowing +/// subclasses to override one or more methods and add to [patches]. Once the +/// stylesheet has been visited, the migrated contents (based on [patches]) will +/// be stored in [migrator]'s [migrated] map. +/// +/// If [migrateDependencies] is enabled, this visitor will construct and run a +/// new instance of itself (using [newInstance]) each time it encounters an +/// @import or @use rule. +abstract class MigrationVisitor extends RecursiveAstVisitor { + /// The migrator running on this stylesheet. + Migrator get migrator; + + /// The canonical path of the stylesheet being migrated. + String get path; + + /// The stylesheet being migrated. + Stylesheet stylesheet; + + /// The syntax this stylesheet uses. + Syntax syntax; + + /// The patches to be applied to the stylesheet being migrated. + final List patches = []; + + /// Returns a new instance of this MigrationVisitor with the same migrator + /// and [newPath]. + MigrationVisitor newInstance(String newPath); + + /// Runs the migrator and stores the migrated contents in `migrator.migrated`. + void run() { + var contents = File(path).readAsStringSync(); + syntax = Syntax.forPath(path); + stylesheet = Stylesheet.parse(contents, syntax, url: path); + visitStylesheet(stylesheet); + var results = getMigratedContents(); + if (results != null) { + migrator.migrated[path] = results; + } + } + + /// Returns the migrated contents of this file, or null if the file does not + /// change. + /// + /// This will be called by [run] and the results will be stored in + /// `migrator.migrated`. + String getMigratedContents() => patches.isNotEmpty + ? Patch.applyAll(patches.first.selection.file, patches) + : null; + + /// Returns the canonical path of [url] when resolved relative to the current + /// path. + String resolveImportUrl(String url) => + canonicalizePath(p.join(p.dirname(path), url)); + + /// If [migrator.migrateDependencies] is enabled, any dynamic imports within + /// this [node] will be migrated before continuing. + @override + visitImportRule(ImportRule node) { + super.visitImportRule(node); + for (var import in node.imports) { + if (import is DynamicImport) { + if (migrator.migrateDependencies) { + newInstance(resolveImportUrl(import.url)).run(); + } + } + } + } + + /// If [migrator.migrateDependencies] is enabled, this dependency will be + /// migrated before continuing. + @override + visitUseRule(UseRule node) { + super.visitUseRule(node); + if (migrator.migrateDependencies) { + newInstance(resolveImportUrl(node.url.toString())).run(); + } + } +} diff --git a/lib/src/migrator.dart b/lib/src/migrator.dart index 20742e1f..fa76b411 100644 --- a/lib/src/migrator.dart +++ b/lib/src/migrator.dart @@ -4,33 +4,44 @@ // license that can be found in the LICENSE file or at // https://opensource.org/licenses/MIT. -import 'package:args/args.dart'; +import 'dart:io'; + +import 'package:args/command_runner.dart'; import 'package:path/path.dart' as p; -abstract class Migrator { - /// Name passed at the command line to use this migrator. - String get name; +import 'utils.dart'; - /// Brief description of what this migrator does. - String get description; +/// A migrator is a command the migrates the entrypoints provided to it and +/// (optionally) their dependencies. +/// +/// Migrators should provide their [name], [description], and optionally +/// [aliases]. +/// +/// Subclasses need to implement [migrateFile], which takes an entrypoint, +/// migrates it, and stores the results in [migrated]. If [migrateDependencies] +/// is true, they should also migrate all of that entrypoint's direct and +/// indirect dependencies. +/// +/// Most migrators will want to create a subclass of [MigrationVisitor] and +/// implement [migrateFile] with `MyMigrationVisitor(this, entrypoint).run()`. +abstract class Migrator extends Command> { + /// The entrypoints that this migrator will run from. + List get entrypoints => argResults.rest; - /// Parser for the arguments that this migrator takes. - ArgParser get argParser => ArgParser(); + /// If true, dependencies will be migrated in addition to the entrypoints. + bool get migrateDependencies => globalResults['migrate-deps'] as bool; - /// Set by the executable based on [argParser] and the provided arguments. - ArgResults argResults; + /// Migrated contents of stylesheets that have already been migrated. + final migrated = p.PathMap(); - /// Runs this migrator on [entrypoint], returning a map of migrated contents. - /// - /// This may also migrate dependencies of this entrypoint, depending on the - /// migrator and its configuration. + /// Runs this migrator on [entrypoint] (and its dependencies, if the + /// --migrate-deps flag is passed). /// - /// Files that did not require migration, even if touched by the migrator, + /// Files that did not require any changes, even if touched by the migrator, /// should not be included map of results. - p.PathMap migrateFile(String entrypoint); + void migrateFile(String entrypoint); - /// Runs this migrator on multiple [entrypoints], returning a merged map of - /// migrated contents. + /// Runs this migrator. /// /// Each entrypoint is migrated separately. If a stylesheet is migrated more /// than once, the resulting migrated contents must be the same each time, or @@ -38,14 +49,15 @@ abstract class Migrator { /// /// Entrypoints and dependencies that did not require any changes will not be /// included in the results. - p.PathMap migrateFiles(Iterable entrypoints) { + p.PathMap run() { var allMigrated = p.PathMap(); for (var entrypoint in entrypoints) { - var migrated = migrateFile(entrypoint); + migrated.clear(); + migrateFile(canonicalizePath(p.join(Directory.current.path, entrypoint))); for (var file in migrated.keys) { if (allMigrated.containsKey(file) && migrated[file] != allMigrated[file]) { - throw UnsupportedError( + throw MigrationException( "$file is migrated in more than one way by these entrypoints."); } allMigrated[file] = migrated[file]; diff --git a/lib/src/migrator_base.dart b/lib/src/migrator_base.dart deleted file mode 100644 index b86cb986..00000000 --- a/lib/src/migrator_base.dart +++ /dev/null @@ -1,113 +0,0 @@ -// Copyright 2019 Google LLC -// -// Use of this source code is governed by an MIT-style -// license that can be found in the LICENSE file or at -// https://opensource.org/licenses/MIT. - -import 'dart:io'; - -// The sass package's API is not necessarily stable. It is being imported with -// the Sass team's explicit knowledge and approval. See -// https://github.com/sass/dart-sass/issues/236. -import 'package:sass/src/ast/sass.dart'; -import 'package:sass/src/syntax.dart'; -import 'package:sass/src/visitor/recursive_statement.dart'; -import 'package:sass/src/visitor/interface/expression.dart'; - -import 'package:path/path.dart' as p; - -import 'migrator.dart'; -import 'patch.dart'; -import 'utils.dart'; - -/// This base migrator parses the stylesheet at each provided entrypoint and -/// recursively visits every statement and expression it contains. -/// -/// Migrators based on this should add appropriate patches to [patches] in -/// overridden methods. -/// -/// On its own, this migrator only touches the entrypoints that are directly -/// provided to it; it does not migrate dependencies. -abstract class MigratorBase extends RecursiveStatementVisitor - with Migrator, ExpressionVisitor { - List _patches = []; - - /// The patches to be applied to the stylesheet being migrated. - /// - /// Subclasses that override this should also override [migrateFile]. - List get patches => _patches; - - /// Runs this migrator on [entrypoint]. - /// - /// This will return either a map containing only the migrated contents of - /// [entrypoint], or an empty map if no migration was necessary. - p.PathMap migrateFile(String entrypoint) { - _patches = []; - var path = canonicalizePath(p.join(Directory.current.path, entrypoint)); - var contents = File(path).readAsStringSync(); - var syntax = Syntax.forPath(path); - visitStylesheet(Stylesheet.parse(contents, syntax, url: path)); - - var results = p.PathMap(); - if (_patches.isNotEmpty) { - results[path] = Patch.applyAll(_patches.first.selection.file, _patches); - } - _patches = null; - return results; - } - - // Expression Tree Traversal - - @override - visitExpression(Expression expression) => expression.accept(this); - - visitBinaryOperationExpression(BinaryOperationExpression node) { - node.left.accept(this); - node.right.accept(this); - } - - visitFunctionExpression(FunctionExpression node) { - visitInterpolation(node.name); - visitArgumentInvocation(node.arguments); - } - - visitIfExpression(IfExpression node) { - visitArgumentInvocation(node.arguments); - } - - visitListExpression(ListExpression node) { - for (var item in node.contents) { - item.accept(this); - } - } - - visitMapExpression(MapExpression node) { - for (var pair in node.pairs) { - pair.item1.accept(this); - pair.item2.accept(this); - } - } - - visitParenthesizedExpression(ParenthesizedExpression node) { - node.expression.accept(this); - } - - visitStringExpression(StringExpression node) { - visitInterpolation(node.text); - } - - visitUnaryOperationExpression(UnaryOperationExpression node) { - node.operand.accept(this); - } - - // Expression Leaves - - visitBooleanExpression(BooleanExpression node) {} - visitColorExpression(ColorExpression node) {} - visitNullExpression(NullExpression node) {} - visitNumberExpression(NumberExpression node) {} - visitSelectorExpression(SelectorExpression node) {} - visitValueExpression(ValueExpression node) {} - visitVariableExpression(VariableExpression node) {} - visitUseRule(UseRule node) {} -} diff --git a/lib/src/migrators/module.dart b/lib/src/migrators/module.dart index 9f2fb967..583adf45 100644 --- a/lib/src/migrators/module.dart +++ b/lib/src/migrators/module.dart @@ -4,38 +4,27 @@ // license that can be found in the LICENSE file or at // https://opensource.org/licenses/MIT. -import 'dart:io'; - // The sass package's API is not necessarily stable. It is being imported with // the Sass team's explicit knowledge and approval. See // https://github.com/sass/dart-sass/issues/236. import 'package:sass/src/ast/sass.dart'; +import 'package:sass/src/syntax.dart'; -import 'package:args/args.dart'; import 'package:path/path.dart' as p; -import 'package:sass_migrator/src/migrator_base.dart'; +import 'package:sass_migrator/src/migration_visitor.dart'; +import 'package:sass_migrator/src/migrator.dart'; import 'package:sass_migrator/src/patch.dart'; import 'package:sass_migrator/src/utils.dart'; import 'module/built_in_functions.dart'; import 'module/local_scope.dart'; -import 'module/stylesheet_migration.dart'; -class ModuleMigrator extends MigratorBase { +/// Migrates stylesheets to the new module system. +class ModuleMigrator extends Migrator { final name = "module"; + final aliases = ["modules", "module-system"]; final description = "Migrates stylesheets to the new module system."; - final argParser = ArgParser() - ..addFlag('migrate-deps', - abbr: 'd', help: 'Migrate dependencies in addition to entrypoints.'); - - bool get _migrateDependencies => argResults['migrate-deps']; - - /// List of all migrations for files touched by this run. - final _migrations = p.PathMap(); - - /// List of migrations in progress. The last item is the current migration. - final _activeMigrations = []; /// Global variables defined at any time during the migrator run. final _variables = normalizedMap(); @@ -46,54 +35,55 @@ class ModuleMigrator extends MigratorBase { /// Global functions defined at any time during the migrator run. final _functions = normalizedMap(); - /// Local variables, mixins, and functions for migrations in progress. - /// - /// The migrator will modify this as it traverses stylesheets. When at the - /// top level of a stylesheet, this will be null. - LocalScope _localScope; - - /// Current stylesheet being actively migrated. - StylesheetMigration get _currentMigration => - _activeMigrations.isNotEmpty ? _activeMigrations.last : null; - - /// The patches to be aplied to the stylesheet being actively migrated. - List get patches => _currentMigration.patches; - /// Runs the module migrator on [entrypoint] and its dependencies and returns /// a map of migrated contents. /// - /// If [_migrateDependencies] is false, the migrator will still be run on + /// If [migrateDependencies] is false, the migrator will still be run on /// dependencies, but they will be excluded from the resulting map. - @override - p.PathMap migrateFile(String entrypoint) { - var migration = _migrateStylesheet(entrypoint, Directory.current.path); - var results = p.PathMap(); - addMigration(StylesheetMigration migration) { - var migrated = migration.migratedContents; - if (migrated != migration.contents) { - results[migration.path] = migrated; - } - } - - if (_migrateDependencies) { - _migrations.values.forEach(addMigration); - } else { - addMigration(migration); + void migrateFile(String entrypoint) { + _ModuleMigrationVisitor(this, entrypoint).run(); + if (!migrateDependencies) { + migrated.removeWhere((path, contents) => path != entrypoint); } - return results; } +} - /// Migrates the stylesheet at [path] if it hasn't already been migrated and - /// returns the StylesheetMigration instance for it regardless. - StylesheetMigration _migrateStylesheet(String path, String directory) { - path = canonicalizePath(p.join(directory, path)); - return _migrations.putIfAbsent(path, () { - var migration = StylesheetMigration(path); - _activeMigrations.add(migration); - visitStylesheet(migration.stylesheet); - _activeMigrations.remove(migration); - return migration; - }); +class _ModuleMigrationVisitor extends MigrationVisitor { + final ModuleMigrator migrator; + final String path; + + _ModuleMigrationVisitor(this.migrator, this.path); + + _ModuleMigrationVisitor newInstance(String newPath) => + _ModuleMigrationVisitor(migrator, newPath); + + /// Namespaces of modules used in this stylesheet. + final namespaces = p.PathMap(); + + /// Set of additional use rules necessary for referencing members of + /// implicit dependencies / built-in modules. + /// + /// This set contains the path provided in the use rule, not the canonical + /// path (e.g. "a" rather than "dir/a.scss"). + final additionalUseRules = Set(); + + /// Global variables declared with !default that could be configured. + final configurableVariables = normalizedSet(); + + /// Local variables, mixins, and functions for this migration. + /// + /// When at the top level of the stylesheet, this will be null. + LocalScope localScope; + + /// Returns the migrated contents of this stylesheet, based on [patches] and + /// [additionalUseRules], or null if the stylesheet does not change. + @override + String getMigratedContents() { + if (patches.isEmpty && additionalUseRules.isEmpty) return null; + var semicolon = syntax == Syntax.sass ? "" : ";"; + var uses = additionalUseRules.map((use) => '@use "$use"$semicolon\n'); + var contents = Patch.applyAll(stylesheet.span.file, patches); + return uses.join("") + contents; } /// Visits the children of [node] with a local scope. @@ -106,16 +96,17 @@ class ModuleMigrator extends MigratorBase { super.visitChildren(node); return; } - _localScope = LocalScope(_localScope); + localScope = LocalScope(localScope); super.visitChildren(node); - _localScope = _localScope.parent; + localScope = localScope.parent; } /// Adds a namespace to any function call that require it. + @override void visitFunctionExpression(FunctionExpression node) { visitInterpolation(node.name); - _patchNamespaceForFunction(node.name.asPlain, (name, namespace) { - _currentMigration.patches.add(Patch(node.name.span, "$namespace.$name")); + patchNamespaceForFunction(node.name.asPlain, (name, namespace) { + patches.add(Patch(node.name.span, "$namespace.$name")); }); visitArgumentInvocation(node.arguments); @@ -129,15 +120,14 @@ class ModuleMigrator extends MigratorBase { return; } var fnName = nameArgument as StringExpression; - _patchNamespaceForFunction(fnName.text.asPlain, (name, namespace) { + patchNamespaceForFunction(fnName.text.asPlain, (name, namespace) { var span = fnName.span; if (fnName.hasQuotes) { span = span.file.span(span.start.offset + 1, span.end.offset - 1); } - _currentMigration.patches.add(Patch(span, name)); + patches.add(Patch(span, name)); var beforeParen = node.span.end.offset - 1; - _currentMigration.patches.add(Patch( - node.span.file.span(beforeParen, beforeParen), + patches.add(Patch(node.span.file.span(beforeParen, beforeParen), ', \$module: "$namespace"')); }); } @@ -150,13 +140,13 @@ class ModuleMigrator extends MigratorBase { /// when namespaced, and the namespace itself. The name will match the name /// provided to the outer function except for built-in functions whose name /// within a module differs from its original name. - void _patchNamespaceForFunction( + void patchNamespaceForFunction( String name, void patcher(String name, String namespace)) { if (name == null) return; - if (_localScope?.isLocalFunction(name) ?? false) return; + if (localScope?.isLocalFunction(name) ?? false) return; - var namespace = _functions.containsKey(name) - ? _currentMigration.namespaceForNode(_functions[name]) + var namespace = migrator._functions.containsKey(name) + ? namespaceForNode(migrator._functions[name]) : null; if (namespace == null) { @@ -164,7 +154,7 @@ class ModuleMigrator extends MigratorBase { namespace = builtInFunctionModules[name]; name = builtInFunctionNameChanges[name] ?? name; - _currentMigration.additionalUseRules.add("sass:$namespace"); + additionalUseRules.add("sass:$namespace"); } if (namespace != null) patcher(name, namespace); } @@ -172,11 +162,12 @@ class ModuleMigrator extends MigratorBase { /// Declares the function within the current scope before visiting it. @override void visitFunctionRule(FunctionRule node) { - _declareFunction(node); + declareFunction(node); super.visitFunctionRule(node); } /// Migrates @import to @use after migrating the imported file. + @override void visitImportRule(ImportRule node) { if (node.imports.first is StaticImport) { super.visitImportRule(node); @@ -188,22 +179,20 @@ class ModuleMigrator extends MigratorBase { } var import = node.imports.first as DynamicImport; - if (_localScope != null) { + if (localScope != null) { // TODO(jathak): Handle nested imports return; } // TODO(jathak): Confirm that this import appears before other rules - - var importMigration = - _migrateStylesheet(import.url, p.dirname(_currentMigration.path)); - _currentMigration.namespaces[importMigration.path] = - namespaceForPath(import.url); + var newPath = resolveImportUrl(import.url); + var importMigration = newInstance(newPath)..run(); + namespaces[importMigration.path] = namespaceForPath(import.url); var overrides = []; for (var variable in importMigration.configurableVariables) { - if (_variables.containsKey(variable)) { - var declaration = _variables[variable]; - if (_currentMigration.namespaceForNode(declaration) == null) { + if (migrator._variables.containsKey(variable)) { + var declaration = migrator._variables[variable]; + if (namespaceForNode(declaration) == null) { overrides.add("\$${declaration.name}: ${declaration.expression}"); } // TODO(jathak): Remove this declaration from the current stylesheet if @@ -214,89 +203,107 @@ class ModuleMigrator extends MigratorBase { if (overrides.isNotEmpty) { config = " with (\n " + overrides.join(',\n ') + "\n)"; } - _currentMigration.patches - .add(Patch(node.span, '@use ${import.span.text}$config')); + patches.add(Patch(node.span, '@use ${import.span.text}$config')); } /// Adds a namespace to any mixin include that requires it. @override void visitIncludeRule(IncludeRule node) { super.visitIncludeRule(node); - if (_localScope?.isLocalMixin(node.name) ?? false) return; - if (!_mixins.containsKey(node.name)) return; - var namespace = _currentMigration.namespaceForNode(_mixins[node.name]); + if (localScope?.isLocalMixin(node.name) ?? false) return; + if (!migrator._mixins.containsKey(node.name)) return; + var namespace = namespaceForNode(migrator._mixins[node.name]); if (namespace == null) return; var endName = node.arguments.span.start.offset; var startName = endName - node.name.length; var nameSpan = node.span.file.span(startName, endName); - _currentMigration.patches.add(Patch(nameSpan, "$namespace.${node.name}")); + patches.add(Patch(nameSpan, "$namespace.${node.name}")); } /// Declares the mixin within the current scope before visiting it. @override void visitMixinRule(MixinRule node) { - _declareMixin(node); + declareMixin(node); super.visitMixinRule(node); } @override void visitUseRule(UseRule node) { // TODO(jathak): Handle existing @use rules. + throw UnsupportedError( + "Migrating files with existing @use rules is not yet supported"); } /// Adds a namespace to any variable that requires it. + @override visitVariableExpression(VariableExpression node) { - if (_localScope?.isLocalVariable(node.name) ?? false) { + if (localScope?.isLocalVariable(node.name) ?? false) { return; } - if (!_variables.containsKey(node.name)) return; - var namespace = _currentMigration.namespaceForNode(_variables[node.name]); + if (!migrator._variables.containsKey(node.name)) return; + var namespace = namespaceForNode(migrator._variables[node.name]); if (namespace == null) return; - _currentMigration.patches - .add(Patch(node.span, "\$$namespace.${node.name}")); + patches.add(Patch(node.span, "\$$namespace.${node.name}")); } /// Declares a variable within the current scope before visiting it. @override void visitVariableDeclaration(VariableDeclaration node) { - _declareVariable(node); + declareVariable(node); super.visitVariableDeclaration(node); } /// Declares a variable within this stylesheet, in the current local scope if /// it exists, or as a global variable otherwise. - void _declareVariable(VariableDeclaration node) { - if (_localScope == null || node.isGlobal) { + void declareVariable(VariableDeclaration node) { + if (localScope == null || node.isGlobal) { if (node.isGuarded) { - _currentMigration.configurableVariables.add(node.name); + configurableVariables.add(node.name); // Don't override if variable already exists. - _variables.putIfAbsent(node.name, () => node); + migrator._variables.putIfAbsent(node.name, () => node); } else { - _variables[node.name] = node; + migrator._variables[node.name] = node; } } else { - _localScope.variables.add(node.name); + localScope.variables.add(node.name); } } /// Declares a mixin within this stylesheet, in the current local scope if /// it exists, or as a global mixin otherwise. - void _declareMixin(MixinRule node) { - if (_localScope == null) { - _mixins[node.name] = node; + void declareMixin(MixinRule node) { + if (localScope == null) { + migrator._mixins[node.name] = node; } else { - _localScope.mixins.add(node.name); + localScope.mixins.add(node.name); } } /// Declares a function within this stylesheet, in the current local scope if /// it exists, or as a global function otherwise. - void _declareFunction(FunctionRule node) { - if (_localScope == null) { - _functions[node.name] = node; + void declareFunction(FunctionRule node) { + if (localScope == null) { + migrator._functions[node.name] = node; } else { - _localScope.functions.add(node.name); + localScope.functions.add(node.name); + } + } + + /// Finds the namespace for the stylesheet containing [node], adding a new use + /// rule if necessary. + String namespaceForNode(SassNode node) { + var nodePath = p.fromUri(node.span.sourceUrl); + if (p.equals(nodePath, path)) return null; + if (!namespaces.containsKey(nodePath)) { + /// Add new use rule for indirect dependency + var relativePath = p.relative(nodePath, from: p.dirname(path)); + var basename = p.basenameWithoutExtension(relativePath); + if (basename.startsWith('_')) basename = basename.substring(1); + var simplePath = p.relative(p.join(p.dirname(relativePath), basename)); + additionalUseRules.add(simplePath); + namespaces[nodePath] = namespaceForPath(simplePath); } + return namespaces[nodePath]; } } diff --git a/lib/src/migrators/module/stylesheet_migration.dart b/lib/src/migrators/module/stylesheet_migration.dart deleted file mode 100644 index 68c7ba4d..00000000 --- a/lib/src/migrators/module/stylesheet_migration.dart +++ /dev/null @@ -1,85 +0,0 @@ -// Copyright 2019 Google LLC -// -// Use of this source code is governed by an MIT-style -// license that can be found in the LICENSE file or at -// https://opensource.org/licenses/MIT. - -import 'dart:io'; - -import 'package:path/path.dart' as p; - -// The sass package's API is not necessarily stable. It is being imported with -// the Sass team's explicit knowledge and approval. See -// https://github.com/sass/dart-sass/issues/236. -import 'package:sass/src/ast/sass.dart'; -import 'package:sass/src/syntax.dart'; - -import 'package:sass_migrator/src/patch.dart'; -import 'package:sass_migrator/src/utils.dart'; - -/// Represents an in-progress migration for a stylesheet. -class StylesheetMigration { - /// The stylesheet this migration is for. - final Stylesheet stylesheet; - - /// The canonical path of this stylesheet. - final String path; - - /// The original contents of this stylesheet, prior to migration. - final String contents; - - /// The syntax used in this stylesheet. - final Syntax syntax; - - /// Namespaces of modules used in this stylesheet. - final namespaces = p.PathMap(); - - /// Set of additional use rules necessary for referencing members of - /// implicit dependencies / built-in modules. - /// - /// This set contains the path provided in the use rule, not the canonical - /// path (e.g. "a" rather than "dir/a.scss"). - final additionalUseRules = Set(); - - /// List of patches to be applied to this file. - final patches = []; - - /// Global variables declared with !default that could be configured. - final configurableVariables = normalizedSet(); - - StylesheetMigration._(this.stylesheet, this.path, this.contents, this.syntax); - - /// Creates a new migration for the stylesheet at [path]. - factory StylesheetMigration(String path) { - var contents = File(path).readAsStringSync(); - var syntax = Syntax.forPath(path); - var stylesheet = Stylesheet.parse(contents, syntax, url: path); - return StylesheetMigration._(stylesheet, path, contents, syntax); - } - - /// Returns the migrated contents of this file, based on [additionalUseRules] - /// and [patches]. - String get migratedContents { - var semicolon = syntax == Syntax.sass ? "" : ";"; - var uses = additionalUseRules.map((use) => '@use "$use"$semicolon\n'); - var contents = Patch.applyAll(stylesheet.span.file, patches); - return uses.join("") + contents; - } - - /// Finds the namespace for the stylesheet containing [node], adding a new use - /// rule if necessary. - String namespaceForNode(SassNode node) { - var nodePath = p.fromUri(node.span.sourceUrl); - if (p.equals(nodePath, path)) return null; - if (!namespaces.containsKey(nodePath)) { - /// Add new use rule for indirect dependency - var relativePath = p.relative(nodePath, from: p.dirname(path)); - var basename = p.basenameWithoutExtension(relativePath); - if (basename.startsWith('_')) basename = basename.substring(1); - var simplePath = p.relative(p.join(p.dirname(relativePath), basename)); - additionalUseRules.add(simplePath); - namespaces[nodePath] = namespaceForPath(simplePath); - } - return namespaces[nodePath]; - } -} diff --git a/lib/src/stylesheet_migration.dart b/lib/src/stylesheet_migration.dart deleted file mode 100644 index 161970de..00000000 --- a/lib/src/stylesheet_migration.dart +++ /dev/null @@ -1,86 +0,0 @@ -// Copyright 2019 Google LLC -// -// Use of this source code is governed by an MIT-style -// license that can be found in the LICENSE file or at -// https://opensource.org/licenses/MIT. - -import 'dart:io'; - -import 'package:path/path.dart' as p; - -// The sass package's API is not necessarily stable. It is being imported with -// the Sass team's explicit knowledge and approval. See -// https://github.com/sass/dart-sass/issues/236. -import 'package:sass/src/ast/sass.dart'; -import 'package:sass/src/syntax.dart'; - -import 'patch.dart'; -import 'utils.dart'; - -/// Represents an in-progress migration for a stylesheet. -class StylesheetMigration { - /// The stylesheet this migration is for. - final Stylesheet stylesheet; - - /// The canonical path of this stylesheet. - final String path; - - /// The original contents of this stylesheet, prior to migration. - final String contents; - - /// The syntax used in this stylesheet. - final Syntax syntax; - - /// Namespaces of modules used in this stylesheet. - final namespaces = p.PathMap(); - - /// Set of additional use rules necessary for referencing members of - /// implicit dependencies / built-in modules. - /// - /// This set contains the path provided in the use rule, not the canonical - /// path (e.g. "a" rather than "dir/a.scss"). - final additionalUseRules = Set(); - - /// List of patches to be applied to this file. - final patches = []; - - /// Global variables declared with !default that could be configured. - final configurableVariables = normalizedSet(); - - StylesheetMigration._(this.stylesheet, this.path, this.contents, this.syntax); - - /// Creates a new migration for the stylesheet at [path]. - factory StylesheetMigration(String path) { - var contents = File(path).readAsStringSync(); - var syntax = Syntax.forPath(path); - var stylesheet = Stylesheet.parse(contents, syntax, url: path); - return StylesheetMigration._(stylesheet, path, contents, syntax); - } - - /// Returns the migrated contents of this file, based on [additionalUseRules] - /// and [patches], or null if no patches exist. - String get migratedContents { - if (patches.isEmpty) return null; - var semicolon = syntax == Syntax.sass ? "" : ";"; - var uses = additionalUseRules.map((use) => '@use "$use"$semicolon\n'); - var contents = Patch.applyAll(stylesheet.span.file, patches); - return uses.join("") + contents; - } - - /// Finds the namespace for the stylesheet containing [node], adding a new use - /// rule if necessary. - String namespaceForNode(SassNode node) { - var nodePath = p.fromUri(node.span.sourceUrl); - if (p.equals(nodePath, path)) return null; - if (!namespaces.containsKey(nodePath)) { - /// Add new use rule for indirect dependency - var relativePath = p.relative(nodePath, from: p.dirname(path)); - var basename = p.basenameWithoutExtension(relativePath); - if (basename.startsWith('_')) basename = basename.substring(1); - var simplePath = p.relative(p.join(p.dirname(relativePath), basename)); - additionalUseRules.add(simplePath); - namespaces[nodePath] = namespaceForPath(simplePath); - } - return namespaces[nodePath]; - } -} diff --git a/lib/src/utils.dart b/lib/src/utils.dart index b29f77e7..957316bb 100644 --- a/lib/src/utils.dart +++ b/lib/src/utils.dart @@ -43,3 +43,18 @@ Patch patchAfter(AstNode node, String text) { void warn(String message, FileSpan context) { print(context.message("WARNING - $message")); } + +class MigrationException { + final String message; + final FileSpan context; + + MigrationException(this.message, {this.context}); + + toString() { + if (context != null) { + return context.message(message); + } else { + return message; + } + } +} diff --git a/pubspec.yaml b/pubspec.yaml index 7c95e5d6..605d1c1d 100644 --- a/pubspec.yaml +++ b/pubspec.yaml @@ -6,8 +6,10 @@ environment: sdk: '>=2.1.0 <3.0.0' dependencies: - args: "^1.4.0" - sass: "^1.17.3" + args: "^1.5.1" + sass: + # Temporary, until 1.17.5 is on pub + git: git://github.com/sass/dart-sass source_span: "^1.4.0" path: "^1.6.0" diff --git a/test/migrator_test.dart b/test/migrator_test.dart index 751ff9ce..958b91b3 100644 --- a/test/migrator_test.dart +++ b/test/migrator_test.dart @@ -6,8 +6,7 @@ import 'dart:io'; -import 'package:sass_migrator/src/migrator.dart'; -import 'package:sass_migrator/src/migrators/module.dart'; +import 'package:sass_migrator/runner.dart'; import 'package:path/path.dart' as p; import 'package:term_glyph/term_glyph.dart' as glyph; @@ -17,12 +16,12 @@ import 'package:test_descriptor/test_descriptor.dart' as d; /// Runs all migration tests. See migrations/README.md for details. void main() { glyph.ascii = true; - testMigrator(ModuleMigrator()); + testMigrator("module"); } -void testMigrator(Migrator migrator) { - var migrationTests = Directory("test/migrations/${migrator.name}"); - group(migrator.name, () { +void testMigrator(String migrator) { + var migrationTests = Directory("test/migrations/$migrator"); + group(migrator, () { for (var file in migrationTests.listSync().whereType()) { if (file.path.endsWith(".hrx")) { test(p.basenameWithoutExtension(file.path), @@ -33,18 +32,18 @@ void testMigrator(Migrator migrator) { } /// Run the migration test in [hrxFile]. See migrations/README.md for details. -testHrx(File hrxFile, Migrator migrator) async { +testHrx(File hrxFile, String migrator) async { var files = HrxTestFiles(hrxFile.readAsStringSync()); await files.unpack(); + p.PathMap migrated; var entrypoints = files.input.keys.where((path) => path.startsWith("entrypoint")); - p.PathMap migrated; - migrator.argResults = migrator.argParser.parse(files.arguments); - expect(() { - IOOverrides.runZoned(() { - migrated = migrator.migrateFiles(entrypoints); - }, getCurrentDirectory: () => Directory(d.sandbox)); - }, prints(files.expectedLog?.replaceAll("\$TEST_DIR", d.sandbox) ?? "")); + var arguments = [migrator]..addAll(files.arguments)..addAll(entrypoints); + await expect( + () => IOOverrides.runZoned(() async { + migrated = await MigratorRunner().run(arguments); + }, getCurrentDirectory: () => Directory(d.sandbox)), + prints(files.expectedLog?.replaceAll("\$TEST_DIR", d.sandbox) ?? "")); for (var file in files.input.keys) { expect(migrated[p.join(d.sandbox, file)], equals(files.output[file]), reason: 'Incorrect migration of $file.'); From 5a58296716ed23195e8739ca83a6caadeed7a237 Mon Sep 17 00:00:00 2001 From: Jennifer Thakar Date: Thu, 4 Apr 2019 16:52:14 -0700 Subject: [PATCH 3/7] Split migrator tests into separate test files --- ...migrator_test.dart => migrator_utils.dart} | 28 +++++++++---------- test/{migrations => migrators}/README.md | 12 +++++++- .../module/globally_shadowed_variable.hrx | 0 .../module/module_configuration.hrx | 0 .../module/namespace_builtin_functions.hrx | 0 .../module/namespace_function.hrx | 0 .../module/namespace_get_function.hrx | 0 .../module/namespace_indirect_reference.hrx | 0 .../module/namespace_mixin.hrx | 0 .../module/namespace_variable.hrx | 0 .../module/subdirectories.hrx | 0 test/migrators/module_test.dart | 11 ++++++++ 12 files changed, 36 insertions(+), 15 deletions(-) rename test/{migrator_test.dart => migrator_utils.dart} (84%) rename test/{migrations => migrators}/README.md (80%) rename test/{migrations => migrators}/module/globally_shadowed_variable.hrx (100%) rename test/{migrations => migrators}/module/module_configuration.hrx (100%) rename test/{migrations => migrators}/module/namespace_builtin_functions.hrx (100%) rename test/{migrations => migrators}/module/namespace_function.hrx (100%) rename test/{migrations => migrators}/module/namespace_get_function.hrx (100%) rename test/{migrations => migrators}/module/namespace_indirect_reference.hrx (100%) rename test/{migrations => migrators}/module/namespace_mixin.hrx (100%) rename test/{migrations => migrators}/module/namespace_variable.hrx (100%) rename test/{migrations => migrators}/module/subdirectories.hrx (100%) create mode 100644 test/migrators/module_test.dart diff --git a/test/migrator_test.dart b/test/migrator_utils.dart similarity index 84% rename from test/migrator_test.dart rename to test/migrator_utils.dart index 958b91b3..beba29f7 100644 --- a/test/migrator_test.dart +++ b/test/migrator_utils.dart @@ -1,4 +1,4 @@ -// Copyright 2018 Google LLC +// Copyright 2019 Google LLC // // Use of this source code is governed by an MIT-style // license that can be found in the LICENSE file or at @@ -13,27 +13,27 @@ import 'package:term_glyph/term_glyph.dart' as glyph; import 'package:test/test.dart'; import 'package:test_descriptor/test_descriptor.dart' as d; -/// Runs all migration tests. See migrations/README.md for details. -void main() { - glyph.ascii = true; - testMigrator("module"); -} - +/// Runs all tests for [migrator]. +/// +/// HRX files should be stored in `test/migrators//`. void testMigrator(String migrator) { - var migrationTests = Directory("test/migrations/$migrator"); + glyph.ascii = true; + var migrationTests = Directory("test/migrators/$migrator"); group(migrator, () { for (var file in migrationTests.listSync().whereType()) { if (file.path.endsWith(".hrx")) { test(p.basenameWithoutExtension(file.path), - () => testHrx(file, migrator)); + () => _testHrx(file, migrator)); } } }); } -/// Run the migration test in [hrxFile]. See migrations/README.md for details. -testHrx(File hrxFile, String migrator) async { - var files = HrxTestFiles(hrxFile.readAsStringSync()); +/// Run the migration test in [hrxFile]. +/// +/// See migrations/README.md for details. +_testHrx(File hrxFile, String migrator) async { + var files = _HrxTestFiles(hrxFile.readAsStringSync()); await files.unpack(); p.PathMap migrated; var entrypoints = @@ -50,13 +50,13 @@ testHrx(File hrxFile, String migrator) async { } } -class HrxTestFiles { +class _HrxTestFiles { Map input = {}; Map output = {}; List arguments = []; String expectedLog; - HrxTestFiles(String hrxText) { + _HrxTestFiles(String hrxText) { // TODO(jathak): Replace this with an actual HRX parser. String filename; String contents; diff --git a/test/migrations/README.md b/test/migrators/README.md similarity index 80% rename from test/migrations/README.md rename to test/migrators/README.md index fc8ba508..ff7483b7 100644 --- a/test/migrations/README.md +++ b/test/migrators/README.md @@ -1,6 +1,16 @@ # Migration Tests -For each migrator, this folder contains a subdirectory with tests. +Each migrator should have a `_test.dart` file that looks like: + +```dart +import '../migrator_utils.dart'; + +main() { + testMigrator(""); +} +``` + +and a directory `` that contains that migrator's HRX tests. Each set of source files used for a test of a migrator is represented a single [HRX archive](https://github.com/google/hrx). diff --git a/test/migrations/module/globally_shadowed_variable.hrx b/test/migrators/module/globally_shadowed_variable.hrx similarity index 100% rename from test/migrations/module/globally_shadowed_variable.hrx rename to test/migrators/module/globally_shadowed_variable.hrx diff --git a/test/migrations/module/module_configuration.hrx b/test/migrators/module/module_configuration.hrx similarity index 100% rename from test/migrations/module/module_configuration.hrx rename to test/migrators/module/module_configuration.hrx diff --git a/test/migrations/module/namespace_builtin_functions.hrx b/test/migrators/module/namespace_builtin_functions.hrx similarity index 100% rename from test/migrations/module/namespace_builtin_functions.hrx rename to test/migrators/module/namespace_builtin_functions.hrx diff --git a/test/migrations/module/namespace_function.hrx b/test/migrators/module/namespace_function.hrx similarity index 100% rename from test/migrations/module/namespace_function.hrx rename to test/migrators/module/namespace_function.hrx diff --git a/test/migrations/module/namespace_get_function.hrx b/test/migrators/module/namespace_get_function.hrx similarity index 100% rename from test/migrations/module/namespace_get_function.hrx rename to test/migrators/module/namespace_get_function.hrx diff --git a/test/migrations/module/namespace_indirect_reference.hrx b/test/migrators/module/namespace_indirect_reference.hrx similarity index 100% rename from test/migrations/module/namespace_indirect_reference.hrx rename to test/migrators/module/namespace_indirect_reference.hrx diff --git a/test/migrations/module/namespace_mixin.hrx b/test/migrators/module/namespace_mixin.hrx similarity index 100% rename from test/migrations/module/namespace_mixin.hrx rename to test/migrators/module/namespace_mixin.hrx diff --git a/test/migrations/module/namespace_variable.hrx b/test/migrators/module/namespace_variable.hrx similarity index 100% rename from test/migrations/module/namespace_variable.hrx rename to test/migrators/module/namespace_variable.hrx diff --git a/test/migrations/module/subdirectories.hrx b/test/migrators/module/subdirectories.hrx similarity index 100% rename from test/migrations/module/subdirectories.hrx rename to test/migrators/module/subdirectories.hrx diff --git a/test/migrators/module_test.dart b/test/migrators/module_test.dart new file mode 100644 index 00000000..eacd22b6 --- /dev/null +++ b/test/migrators/module_test.dart @@ -0,0 +1,11 @@ +// Copyright 2019 Google LLC +// +// Use of this source code is governed by an MIT-style +// license that can be found in the LICENSE file or at +// https://opensource.org/licenses/MIT. + +import '../migrator_utils.dart'; + +main() { + testMigrator("module"); +} From 28c09b89a0615a3c2747c0e1d7049ab189557c99 Mon Sep 17 00:00:00 2001 From: Jennifer Thakar Date: Thu, 4 Apr 2019 17:26:26 -0700 Subject: [PATCH 4/7] Minor formatting fixes --- CONTRIBUTING.md | 2 +- lib/runner.dart | 2 +- lib/src/migrator.dart | 2 ++ lib/src/migrators/module.dart | 2 +- lib/src/migrators/module/built_in_functions.dart | 6 ++++++ lib/src/patch.dart | 2 +- test/patch_test.dart | 2 +- 7 files changed, 13 insertions(+), 5 deletions(-) diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index 7f5c3bbe..0059d725 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -23,7 +23,7 @@ All submissions, including submissions by project members, require review. ### File headers All files in the project must start with the following header. - // Copyright 2018 Google LLC + // Copyright 2019 Google LLC // // Use of this source code is governed by an MIT-style // license that can be found in the LICENSE file or at diff --git a/lib/runner.dart b/lib/runner.dart index 9bf8b912..2f37e240 100644 --- a/lib/runner.dart +++ b/lib/runner.dart @@ -13,7 +13,7 @@ import 'src/migrators/module.dart'; /// A command runner that runs a migrator based on provided arguments. class MigratorRunner extends CommandRunner> { - final invocation = "sass_migrator [options] "; + final invocation = "sass_migrator [options] "; MigratorRunner() : super("sass_migrator", "Migrates stylesheets to new Sass versions.") { diff --git a/lib/src/migrator.dart b/lib/src/migrator.dart index fa76b411..24d1c291 100644 --- a/lib/src/migrator.dart +++ b/lib/src/migrator.dart @@ -7,6 +7,7 @@ import 'dart:io'; import 'package:args/command_runner.dart'; +import 'package:meta/meta.dart'; import 'package:path/path.dart' as p; import 'utils.dart'; @@ -39,6 +40,7 @@ abstract class Migrator extends Command> { /// /// Files that did not require any changes, even if touched by the migrator, /// should not be included map of results. + @protected void migrateFile(String entrypoint); /// Runs this migrator. diff --git a/lib/src/migrators/module.dart b/lib/src/migrators/module.dart index 583adf45..e1e7793b 100644 --- a/lib/src/migrators/module.dart +++ b/lib/src/migrators/module.dart @@ -1,4 +1,4 @@ -// Copyright 2018 Google LLC +// Copyright 2019 Google LLC // // Use of this source code is governed by an MIT-style // license that can be found in the LICENSE file or at diff --git a/lib/src/migrators/module/built_in_functions.dart b/lib/src/migrators/module/built_in_functions.dart index 72f9677e..6ffd8ad9 100644 --- a/lib/src/migrators/module/built_in_functions.dart +++ b/lib/src/migrators/module/built_in_functions.dart @@ -1,3 +1,9 @@ +// Copyright 2019 Google LLC +// +// Use of this source code is governed by an MIT-style +// license that can be found in the LICENSE file or at +// https://opensource.org/licenses/MIT. + /// Mapping from existing built-in function name to the module it's now part of. const builtInFunctionModules = { "red": "color", diff --git a/lib/src/patch.dart b/lib/src/patch.dart index b4742537..bc9ea133 100644 --- a/lib/src/patch.dart +++ b/lib/src/patch.dart @@ -1,4 +1,4 @@ -// Copyright 2018 Google LLC +// Copyright 2019 Google LLC // // Use of this source code is governed by an MIT-style // license that can be found in the LICENSE file or at diff --git a/test/patch_test.dart b/test/patch_test.dart index 5c1aee9f..e2acff07 100644 --- a/test/patch_test.dart +++ b/test/patch_test.dart @@ -1,4 +1,4 @@ -// Copyright 2018 Google LLC +// Copyright 2019 Google LLC // // Use of this source code is governed by an MIT-style // license that can be found in the LICENSE file or at From 0b6a06120605408ed6f2ad6bb5898b1e14771890 Mon Sep 17 00:00:00 2001 From: Jennifer Thakar Date: Mon, 8 Apr 2019 16:08:05 -0700 Subject: [PATCH 5/7] Iterate in response to review. The migrator now uses URLs instead of paths, and loads files using the FilesystemImporter. `Migrator.migrated` has been removed in favor of having `Migrator.migrateFile` and `MigrationVisitor.run` return it. --- lib/runner.dart | 22 ++++--- lib/src/migration_visitor.dart | 80 ++++++++++++++----------- lib/src/migrator.dart | 24 +++----- lib/src/migrators/module.dart | 106 +++++++++++++++++---------------- lib/src/utils.dart | 34 +++++++---- pubspec.yaml | 4 +- test/migrator_utils.dart | 12 +++- 7 files changed, 153 insertions(+), 129 deletions(-) diff --git a/lib/runner.dart b/lib/runner.dart index 2f37e240..01662a97 100644 --- a/lib/runner.dart +++ b/lib/runner.dart @@ -7,12 +7,11 @@ import 'dart:io'; import 'package:args/command_runner.dart'; -import 'package:path/path.dart' as p; import 'src/migrators/module.dart'; /// A command runner that runs a migrator based on provided arguments. -class MigratorRunner extends CommandRunner> { +class MigratorRunner extends CommandRunner> { final invocation = "sass_migrator [options] "; MigratorRunner() @@ -22,6 +21,7 @@ class MigratorRunner extends CommandRunner> { argParser.addFlag('dry-run', abbr: 'n', help: 'Show which files would be migrated but make no changes.'); + // TODO(jathak): Make this flag print a diff instead. argParser.addFlag('verbose', abbr: 'v', help: 'Print text of migrated files when running with --dry-run.'); @@ -29,7 +29,7 @@ class MigratorRunner extends CommandRunner> { } /// Runs a migrator and then writes the migrated files to disk unless - /// --dry-run is passed. + /// `--dry-run` is passed. Future execute(Iterable args) async { var argResults = parse(args); var migrated = await runCommand(argResults); @@ -42,17 +42,21 @@ class MigratorRunner extends CommandRunner> { if (argResults['dry-run']) { print('Dry run. Logging migrated files instead of overwriting...\n'); - for (var path in migrated.keys) { - print('$path'); + for (var url in migrated.keys) { + print('$url'); if (argResults['verbose']) { print('=' * 80); - print(migrated[path]); + print(migrated[url]); + print('-' * 80); } } } else { - for (var path in migrated.keys) { - if (argResults['verbose']) print("Overwriting $path..."); - File(path).writeAsStringSync(migrated[path]); + for (var url in migrated.keys) { + if (url.scheme != null && url.scheme != "file") { + print("Cannot write to $url"); + } + if (argResults['verbose']) print("Overwriting $url..."); + File(url.toFilePath()).writeAsStringSync(migrated[url]); } } } diff --git a/lib/src/migration_visitor.dart b/lib/src/migration_visitor.dart index 10d823e4..523240a5 100644 --- a/lib/src/migration_visitor.dart +++ b/lib/src/migration_visitor.dart @@ -4,18 +4,16 @@ // license that can be found in the LICENSE file or at // https://opensource.org/licenses/MIT. -import 'dart:io'; +import 'dart:collection'; // The sass package's API is not necessarily stable. It is being imported with // the Sass team's explicit knowledge and approval. See // https://github.com/sass/dart-sass/issues/236. import 'package:sass/src/ast/sass.dart'; -import 'package:sass/src/syntax.dart'; import 'package:sass/src/visitor/recursive_ast.dart'; -import 'package:path/path.dart' as p; +import 'package:meta/meta.dart'; -import 'migrator.dart'; import 'patch.dart'; import 'utils.dart'; @@ -28,37 +26,46 @@ import 'utils.dart'; /// /// If [migrateDependencies] is enabled, this visitor will construct and run a /// new instance of itself (using [newInstance]) each time it encounters an -/// @import or @use rule. +/// `@import` or `@use` rule. abstract class MigrationVisitor extends RecursiveAstVisitor { - /// The migrator running on this stylesheet. - Migrator get migrator; - - /// The canonical path of the stylesheet being migrated. - String get path; - /// The stylesheet being migrated. - Stylesheet stylesheet; + final Stylesheet stylesheet; + + /// A mapping from URLs to migrated contents for stylesheets already migrated. + final Map migrated; - /// The syntax this stylesheet uses. - Syntax syntax; + /// True if dependencies should be migrated as well. + final bool migrateDependencies; /// The patches to be applied to the stylesheet being migrated. - final List patches = []; + UnmodifiableListView get patches => UnmodifiableListView(_patches); + final List _patches = []; - /// Returns a new instance of this MigrationVisitor with the same migrator - /// and [newPath]. - MigrationVisitor newInstance(String newPath); + /// Constructs a new migration visitor, parsing the stylesheet at [url]. + MigrationVisitor(Uri url, this.migrateDependencies, + {Map migrated}) + : stylesheet = parseStylesheet(url), + this.migrated = migrated ?? {}; + + /// Returns a new instance of this MigrationVisitor with the same [migrated] + /// and [migrateDependencies] and the new [url]. + @protected + MigrationVisitor newInstance(Uri url); + + /// Adds a new patch that should be applied as part of this migration. + @protected + void addPatch(Patch patch) { + _patches.add(patch); + } - /// Runs the migrator and stores the migrated contents in `migrator.migrated`. - void run() { - var contents = File(path).readAsStringSync(); - syntax = Syntax.forPath(path); - stylesheet = Stylesheet.parse(contents, syntax, url: path); + /// Runs the migrator and returns the map of migrated contents. + Map run() { visitStylesheet(stylesheet); var results = getMigratedContents(); if (results != null) { - migrator.migrated[path] = results; + migrated[stylesheet.span.file.url] = results; } + return migrated; } /// Returns the migrated contents of this file, or null if the file does not @@ -66,36 +73,37 @@ abstract class MigrationVisitor extends RecursiveAstVisitor { /// /// This will be called by [run] and the results will be stored in /// `migrator.migrated`. + @protected String getMigratedContents() => patches.isNotEmpty ? Patch.applyAll(patches.first.selection.file, patches) : null; - /// Returns the canonical path of [url] when resolved relative to the current - /// path. - String resolveImportUrl(String url) => - canonicalizePath(p.join(p.dirname(path), url)); + /// Resolves [relativeUrl] relative to the current stylesheet's URL. + @protected + Uri resolveRelativeUrl(Uri relativeUrl) => + stylesheet.span.file.url.resolveUri(relativeUrl); - /// If [migrator.migrateDependencies] is enabled, any dynamic imports within + /// If [migrateDependencies] is enabled, any dynamic imports within /// this [node] will be migrated before continuing. @override visitImportRule(ImportRule node) { super.visitImportRule(node); - for (var import in node.imports) { - if (import is DynamicImport) { - if (migrator.migrateDependencies) { - newInstance(resolveImportUrl(import.url)).run(); + if (migrateDependencies) { + for (var import in node.imports) { + if (import is DynamicImport) { + newInstance(resolveRelativeUrl(Uri.parse(import.url))).run(); } } } } - /// If [migrator.migrateDependencies] is enabled, this dependency will be + /// If [migrateDependencies] is enabled, this dependency will be /// migrated before continuing. @override visitUseRule(UseRule node) { super.visitUseRule(node); - if (migrator.migrateDependencies) { - newInstance(resolveImportUrl(node.url.toString())).run(); + if (migrateDependencies) { + newInstance(resolveRelativeUrl(node.url)).run(); } } } diff --git a/lib/src/migrator.dart b/lib/src/migrator.dart index 24d1c291..34ee257c 100644 --- a/lib/src/migrator.dart +++ b/lib/src/migrator.dart @@ -4,15 +4,12 @@ // license that can be found in the LICENSE file or at // https://opensource.org/licenses/MIT. -import 'dart:io'; - import 'package:args/command_runner.dart'; import 'package:meta/meta.dart'; -import 'package:path/path.dart' as p; import 'utils.dart'; -/// A migrator is a command the migrates the entrypoints provided to it and +/// A migrator is a command that migrates the entrypoints provided to it and /// (optionally) their dependencies. /// /// Migrators should provide their [name], [description], and optionally @@ -25,23 +22,17 @@ import 'utils.dart'; /// /// Most migrators will want to create a subclass of [MigrationVisitor] and /// implement [migrateFile] with `MyMigrationVisitor(this, entrypoint).run()`. -abstract class Migrator extends Command> { - /// The entrypoints that this migrator will run from. - List get entrypoints => argResults.rest; - +abstract class Migrator extends Command> { /// If true, dependencies will be migrated in addition to the entrypoints. bool get migrateDependencies => globalResults['migrate-deps'] as bool; - /// Migrated contents of stylesheets that have already been migrated. - final migrated = p.PathMap(); - /// Runs this migrator on [entrypoint] (and its dependencies, if the /// --migrate-deps flag is passed). /// /// Files that did not require any changes, even if touched by the migrator, /// should not be included map of results. @protected - void migrateFile(String entrypoint); + Map migrateFile(Uri entrypoint); /// Runs this migrator. /// @@ -51,11 +42,10 @@ abstract class Migrator extends Command> { /// /// Entrypoints and dependencies that did not require any changes will not be /// included in the results. - p.PathMap run() { - var allMigrated = p.PathMap(); - for (var entrypoint in entrypoints) { - migrated.clear(); - migrateFile(canonicalizePath(p.join(Directory.current.path, entrypoint))); + Map run() { + var allMigrated = Map(); + for (var entrypoint in argResults.rest) { + var migrated = migrateFile(canonicalize(Uri.parse(entrypoint))); for (var file in migrated.keys) { if (allMigrated.containsKey(file) && migrated[file] != allMigrated[file]) { diff --git a/lib/src/migrators/module.dart b/lib/src/migrators/module.dart index e1e7793b..1feeeba9 100644 --- a/lib/src/migrators/module.dart +++ b/lib/src/migrators/module.dart @@ -8,7 +8,6 @@ // the Sass team's explicit knowledge and approval. See // https://github.com/sass/dart-sass/issues/236. import 'package:sass/src/ast/sass.dart'; -import 'package:sass/src/syntax.dart'; import 'package:path/path.dart' as p; @@ -23,7 +22,6 @@ import 'module/local_scope.dart'; /// Migrates stylesheets to the new module system. class ModuleMigrator extends Migrator { final name = "module"; - final aliases = ["modules", "module-system"]; final description = "Migrates stylesheets to the new module system."; /// Global variables defined at any time during the migrator run. @@ -40,50 +38,56 @@ class ModuleMigrator extends Migrator { /// /// If [migrateDependencies] is false, the migrator will still be run on /// dependencies, but they will be excluded from the resulting map. - void migrateFile(String entrypoint) { - _ModuleMigrationVisitor(this, entrypoint).run(); + Map migrateFile(Uri entrypoint) { + var migrated = _ModuleMigrationVisitor(this, entrypoint).run(); if (!migrateDependencies) { - migrated.removeWhere((path, contents) => path != entrypoint); + migrated.removeWhere((url, contents) => url != entrypoint); } + return migrated; } } class _ModuleMigrationVisitor extends MigrationVisitor { final ModuleMigrator migrator; - final String path; - _ModuleMigrationVisitor(this.migrator, this.path); + /// Constructs a new module migration visitor. + /// + /// Note: We always set [migratedDependencies] to true since the module + /// migrator needs to always run on dependencies. The `migrateFile` method of + /// the module migrator will filter out the dependencies' migration results. + _ModuleMigrationVisitor(this.migrator, Uri url, {Map migrated}) + : super(url, true, migrated: migrated); - _ModuleMigrationVisitor newInstance(String newPath) => - _ModuleMigrationVisitor(migrator, newPath); + _ModuleMigrationVisitor newInstance(Uri url) => + _ModuleMigrationVisitor(migrator, url, migrated: migrated); /// Namespaces of modules used in this stylesheet. - final namespaces = p.PathMap(); + final _namespaces = {}; /// Set of additional use rules necessary for referencing members of /// implicit dependencies / built-in modules. /// /// This set contains the path provided in the use rule, not the canonical /// path (e.g. "a" rather than "dir/a.scss"). - final additionalUseRules = Set(); + final _additionalUseRules = Set(); /// Global variables declared with !default that could be configured. - final configurableVariables = normalizedSet(); + final _configurableVariables = normalizedSet(); /// Local variables, mixins, and functions for this migration. /// /// When at the top level of the stylesheet, this will be null. - LocalScope localScope; + LocalScope _localScope; /// Returns the migrated contents of this stylesheet, based on [patches] and - /// [additionalUseRules], or null if the stylesheet does not change. + /// [_additionalUseRules], or null if the stylesheet does not change. @override String getMigratedContents() { - if (patches.isEmpty && additionalUseRules.isEmpty) return null; - var semicolon = syntax == Syntax.sass ? "" : ";"; - var uses = additionalUseRules.map((use) => '@use "$use"$semicolon\n'); - var contents = Patch.applyAll(stylesheet.span.file, patches); - return uses.join("") + contents; + var results = super.getMigratedContents(); + if (results == null) return null; + var semicolon = stylesheet.span.file.url.path.endsWith('.sass') ? "" : ";"; + var uses = _additionalUseRules.map((use) => '@use "$use"$semicolon\n'); + return uses.join("") + results; } /// Visits the children of [node] with a local scope. @@ -96,9 +100,9 @@ class _ModuleMigrationVisitor extends MigrationVisitor { super.visitChildren(node); return; } - localScope = LocalScope(localScope); + _localScope = LocalScope(_localScope); super.visitChildren(node); - localScope = localScope.parent; + _localScope = _localScope.parent; } /// Adds a namespace to any function call that require it. @@ -106,7 +110,7 @@ class _ModuleMigrationVisitor extends MigrationVisitor { void visitFunctionExpression(FunctionExpression node) { visitInterpolation(node.name); patchNamespaceForFunction(node.name.asPlain, (name, namespace) { - patches.add(Patch(node.name.span, "$namespace.$name")); + addPatch(Patch(node.name.span, "$namespace.$name")); }); visitArgumentInvocation(node.arguments); @@ -125,9 +129,9 @@ class _ModuleMigrationVisitor extends MigrationVisitor { if (fnName.hasQuotes) { span = span.file.span(span.start.offset + 1, span.end.offset - 1); } - patches.add(Patch(span, name)); + addPatch(Patch(span, name)); var beforeParen = node.span.end.offset - 1; - patches.add(Patch(node.span.file.span(beforeParen, beforeParen), + addPatch(Patch(node.span.file.span(beforeParen, beforeParen), ', \$module: "$namespace"')); }); } @@ -143,7 +147,7 @@ class _ModuleMigrationVisitor extends MigrationVisitor { void patchNamespaceForFunction( String name, void patcher(String name, String namespace)) { if (name == null) return; - if (localScope?.isLocalFunction(name) ?? false) return; + if (_localScope?.isLocalFunction(name) ?? false) return; var namespace = migrator._functions.containsKey(name) ? namespaceForNode(migrator._functions[name]) @@ -151,10 +155,9 @@ class _ModuleMigrationVisitor extends MigrationVisitor { if (namespace == null) { if (!builtInFunctionModules.containsKey(name)) return; - namespace = builtInFunctionModules[name]; name = builtInFunctionNameChanges[name] ?? name; - additionalUseRules.add("sass:$namespace"); + _additionalUseRules.add("sass:$namespace"); } if (namespace != null) patcher(name, namespace); } @@ -179,17 +182,18 @@ class _ModuleMigrationVisitor extends MigrationVisitor { } var import = node.imports.first as DynamicImport; - if (localScope != null) { + if (_localScope != null) { // TODO(jathak): Handle nested imports return; } // TODO(jathak): Confirm that this import appears before other rules - var newPath = resolveImportUrl(import.url); - var importMigration = newInstance(newPath)..run(); - namespaces[importMigration.path] = namespaceForPath(import.url); + var url = resolveRelativeUrl(Uri.parse(import.url)); + var importMigration = newInstance(url)..run(); + _namespaces[importMigration.stylesheet.span.sourceUrl] = + namespaceForPath(import.url); var overrides = []; - for (var variable in importMigration.configurableVariables) { + for (var variable in importMigration._configurableVariables) { if (migrator._variables.containsKey(variable)) { var declaration = migrator._variables[variable]; if (namespaceForNode(declaration) == null) { @@ -203,21 +207,21 @@ class _ModuleMigrationVisitor extends MigrationVisitor { if (overrides.isNotEmpty) { config = " with (\n " + overrides.join(',\n ') + "\n)"; } - patches.add(Patch(node.span, '@use ${import.span.text}$config')); + addPatch(Patch(node.span, '@use ${import.span.text}$config')); } /// Adds a namespace to any mixin include that requires it. @override void visitIncludeRule(IncludeRule node) { super.visitIncludeRule(node); - if (localScope?.isLocalMixin(node.name) ?? false) return; + if (_localScope?.isLocalMixin(node.name) ?? false) return; if (!migrator._mixins.containsKey(node.name)) return; var namespace = namespaceForNode(migrator._mixins[node.name]); if (namespace == null) return; var endName = node.arguments.span.start.offset; var startName = endName - node.name.length; var nameSpan = node.span.file.span(startName, endName); - patches.add(Patch(nameSpan, "$namespace.${node.name}")); + addPatch(Patch(nameSpan, "$namespace.${node.name}")); } /// Declares the mixin within the current scope before visiting it. @@ -237,13 +241,13 @@ class _ModuleMigrationVisitor extends MigrationVisitor { /// Adds a namespace to any variable that requires it. @override visitVariableExpression(VariableExpression node) { - if (localScope?.isLocalVariable(node.name) ?? false) { + if (_localScope?.isLocalVariable(node.name) ?? false) { return; } if (!migrator._variables.containsKey(node.name)) return; var namespace = namespaceForNode(migrator._variables[node.name]); if (namespace == null) return; - patches.add(Patch(node.span, "\$$namespace.${node.name}")); + addPatch(Patch(node.span, "\$$namespace.${node.name}")); } /// Declares a variable within the current scope before visiting it. @@ -256,9 +260,9 @@ class _ModuleMigrationVisitor extends MigrationVisitor { /// Declares a variable within this stylesheet, in the current local scope if /// it exists, or as a global variable otherwise. void declareVariable(VariableDeclaration node) { - if (localScope == null || node.isGlobal) { + if (_localScope == null || node.isGlobal) { if (node.isGuarded) { - configurableVariables.add(node.name); + _configurableVariables.add(node.name); // Don't override if variable already exists. migrator._variables.putIfAbsent(node.name, () => node); @@ -266,44 +270,44 @@ class _ModuleMigrationVisitor extends MigrationVisitor { migrator._variables[node.name] = node; } } else { - localScope.variables.add(node.name); + _localScope.variables.add(node.name); } } /// Declares a mixin within this stylesheet, in the current local scope if /// it exists, or as a global mixin otherwise. void declareMixin(MixinRule node) { - if (localScope == null) { + if (_localScope == null) { migrator._mixins[node.name] = node; } else { - localScope.mixins.add(node.name); + _localScope.mixins.add(node.name); } } /// Declares a function within this stylesheet, in the current local scope if /// it exists, or as a global function otherwise. void declareFunction(FunctionRule node) { - if (localScope == null) { + if (_localScope == null) { migrator._functions[node.name] = node; } else { - localScope.functions.add(node.name); + _localScope.functions.add(node.name); } } /// Finds the namespace for the stylesheet containing [node], adding a new use /// rule if necessary. String namespaceForNode(SassNode node) { - var nodePath = p.fromUri(node.span.sourceUrl); - if (p.equals(nodePath, path)) return null; - if (!namespaces.containsKey(nodePath)) { + if (node.span.sourceUrl == stylesheet.span.sourceUrl) return null; + if (!_namespaces.containsKey(node.span.sourceUrl)) { /// Add new use rule for indirect dependency - var relativePath = p.relative(nodePath, from: p.dirname(path)); + var relativePath = p.relative(node.span.sourceUrl.path, + from: p.dirname(stylesheet.span.sourceUrl.path)); var basename = p.basenameWithoutExtension(relativePath); if (basename.startsWith('_')) basename = basename.substring(1); var simplePath = p.relative(p.join(p.dirname(relativePath), basename)); - additionalUseRules.add(simplePath); - namespaces[nodePath] = namespaceForPath(simplePath); + _additionalUseRules.add(simplePath); + _namespaces[node.span.sourceUrl] = namespaceForPath(simplePath); } - return namespaces[nodePath]; + return _namespaces[node.span.sourceUrl]; } } diff --git a/lib/src/utils.dart b/lib/src/utils.dart index 957316bb..d9b81ec5 100644 --- a/lib/src/utils.dart +++ b/lib/src/utils.dart @@ -4,21 +4,32 @@ // license that can be found in the LICENSE file or at // https://opensource.org/licenses/MIT. -import 'package:path/path.dart' as p; +import 'dart:io'; + import 'package:source_span/source_span.dart'; // The sass package's API is not necessarily stable. It is being imported with // the Sass team's explicit knowledge and approval. See // https://github.com/sass/dart-sass/issues/236. +import 'package:sass/src/ast/sass.dart'; import 'package:sass/src/ast/node.dart'; -import 'package:sass/src/importer/utils.dart' show resolveImportPath; +import 'package:sass/src/importer/filesystem.dart'; export 'package:sass/src/utils.dart' show normalizedMap, normalizedSet; import 'patch.dart'; -/// Returns the canonical version of [path]. -String canonicalizePath(String path) { - return p.canonicalize(resolveImportPath(path)); +/// Returns the canonical version of [url]. +Uri canonicalize(Uri url) { + var importer = FilesystemImporter(Directory.current.path); + return importer.canonicalize(url); +} + +/// Parses the file at [url] into a stylesheet. +Stylesheet parseStylesheet(Uri url) { + var importer = FilesystemImporter(Directory.current.path); + url = importer.canonicalize(url); + var result = importer.load(url); + return Stylesheet.parse(result.contents, result.syntax, url: url); } /// Returns the default namespace for a use rule with [path]. @@ -44,15 +55,18 @@ void warn(String message, FileSpan context) { print(context.message("WARNING - $message")); } +/// An exception thrown by a migrator. class MigrationException { final String message; - final FileSpan context; - MigrationException(this.message, {this.context}); + /// The span that triggered this exception, or null if there is none. + final FileSpan span; + + MigrationException(this.message, {this.span}); - toString() { - if (context != null) { - return context.message(message); + String toString() { + if (span != null) { + return span.message(message); } else { return message; } diff --git a/pubspec.yaml b/pubspec.yaml index 605d1c1d..66f07954 100644 --- a/pubspec.yaml +++ b/pubspec.yaml @@ -7,9 +7,7 @@ environment: dependencies: args: "^1.5.1" - sass: - # Temporary, until 1.17.5 is on pub - git: git://github.com/sass/dart-sass + sass: "^1.18.0" source_span: "^1.4.0" path: "^1.6.0" diff --git a/test/migrator_utils.dart b/test/migrator_utils.dart index beba29f7..5a2b13a5 100644 --- a/test/migrator_utils.dart +++ b/test/migrator_utils.dart @@ -6,13 +6,18 @@ import 'dart:io'; -import 'package:sass_migrator/runner.dart'; +// The sass package's API is not necessarily stable. It is being imported with +// the Sass team's explicit knowledge and approval. See +// https://github.com/sass/dart-sass/issues/236. +import 'package:sass/src/importer/filesystem.dart'; import 'package:path/path.dart' as p; import 'package:term_glyph/term_glyph.dart' as glyph; import 'package:test/test.dart'; import 'package:test_descriptor/test_descriptor.dart' as d; +import 'package:sass_migrator/runner.dart'; + /// Runs all tests for [migrator]. /// /// HRX files should be stored in `test/migrators//`. @@ -35,7 +40,7 @@ void testMigrator(String migrator) { _testHrx(File hrxFile, String migrator) async { var files = _HrxTestFiles(hrxFile.readAsStringSync()); await files.unpack(); - p.PathMap migrated; + Map migrated; var entrypoints = files.input.keys.where((path) => path.startsWith("entrypoint")); var arguments = [migrator]..addAll(files.arguments)..addAll(entrypoints); @@ -44,8 +49,9 @@ _testHrx(File hrxFile, String migrator) async { migrated = await MigratorRunner().run(arguments); }, getCurrentDirectory: () => Directory(d.sandbox)), prints(files.expectedLog?.replaceAll("\$TEST_DIR", d.sandbox) ?? "")); + var canonicalize = FilesystemImporter(d.sandbox).canonicalize; for (var file in files.input.keys) { - expect(migrated[p.join(d.sandbox, file)], equals(files.output[file]), + expect(migrated[canonicalize(Uri.parse(file))], equals(files.output[file]), reason: 'Incorrect migration of $file.'); } } From 8bd5417b85045dfb7d319dca490c0626828e3b36 Mon Sep 17 00:00:00 2001 From: Jennifer Thakar Date: Wed, 17 Apr 2019 16:05:42 -0700 Subject: [PATCH 6/7] Reuse single MigrationVisitor A single MigrationVisitor is now reused instead of constructing a new instance for each dependency. This also removes support for configurable variables, since the previous implementation of this was both hard to implement with a single MigrationVisitor and incorrect when the configurable variables were declared in an indirect dependency. A subsequent PR will add this back with a new implementation that fixes the bug with indirect dependencies. --- lib/runner.dart | 8 +- lib/src/migration_visitor.dart | 63 ++++---- lib/src/migrators/module.dart | 141 +++++++++--------- test/migrator_utils.dart | 5 +- .../migrators/module/module_configuration.hrx | 18 --- 5 files changed, 108 insertions(+), 127 deletions(-) delete mode 100644 test/migrators/module/module_configuration.hrx diff --git a/lib/runner.dart b/lib/runner.dart index 01662a97..6a1510d6 100644 --- a/lib/runner.dart +++ b/lib/runner.dart @@ -7,6 +7,7 @@ import 'dart:io'; import 'package:args/command_runner.dart'; +import 'package:path/path.dart' as p; import 'src/migrators/module.dart'; @@ -43,7 +44,7 @@ class MigratorRunner extends CommandRunner> { if (argResults['dry-run']) { print('Dry run. Logging migrated files instead of overwriting...\n'); for (var url in migrated.keys) { - print('$url'); + print(p.prettyUri(url)); if (argResults['verbose']) { print('=' * 80); print(migrated[url]); @@ -52,9 +53,8 @@ class MigratorRunner extends CommandRunner> { } } else { for (var url in migrated.keys) { - if (url.scheme != null && url.scheme != "file") { - print("Cannot write to $url"); - } + assert(url.scheme == null || url.scheme == "file", + "$url is not a file path."); if (argResults['verbose']) print("Overwriting $url..."); File(url.toFilePath()).writeAsStringSync(migrated[url]); } diff --git a/lib/src/migration_visitor.dart b/lib/src/migration_visitor.dart index 523240a5..195d2a1d 100644 --- a/lib/src/migration_visitor.dart +++ b/lib/src/migration_visitor.dart @@ -28,44 +28,48 @@ import 'utils.dart'; /// new instance of itself (using [newInstance]) each time it encounters an /// `@import` or `@use` rule. abstract class MigrationVisitor extends RecursiveAstVisitor { - /// The stylesheet being migrated. - final Stylesheet stylesheet; - /// A mapping from URLs to migrated contents for stylesheets already migrated. - final Map migrated; + final _migrated = {}; /// True if dependencies should be migrated as well. final bool migrateDependencies; /// The patches to be applied to the stylesheet being migrated. UnmodifiableListView get patches => UnmodifiableListView(_patches); - final List _patches = []; - - /// Constructs a new migration visitor, parsing the stylesheet at [url]. - MigrationVisitor(Uri url, this.migrateDependencies, - {Map migrated}) - : stylesheet = parseStylesheet(url), - this.migrated = migrated ?? {}; + List _patches; - /// Returns a new instance of this MigrationVisitor with the same [migrated] - /// and [migrateDependencies] and the new [url]. - @protected - MigrationVisitor newInstance(Uri url); + MigrationVisitor({this.migrateDependencies = true}); - /// Adds a new patch that should be applied as part of this migration. - @protected - void addPatch(Patch patch) { - _patches.add(patch); + /// Runs a new migration on [url] (and its dependencies, if + /// [migrateDependencies] is true) and returns a map of migrated contents. + Map run(Uri url) { + visitStylesheet(parseStylesheet(url)); + return _migrated; } - /// Runs the migrator and returns the map of migrated contents. - Map run() { - visitStylesheet(stylesheet); + /// Visits stylesheet starting with an empty [_patches], adds the migrated + /// contents (if any) to [_migrated], and then restores the previous value of + /// [_patches]. + /// + /// Migrators with per-file state should override this to store the current + /// file's state before calling the super method and restore it afterwards. + @override + void visitStylesheet(Stylesheet node) { + var oldPatches = _patches; + _patches = []; + super.visitStylesheet(node); var results = getMigratedContents(); if (results != null) { - migrated[stylesheet.span.file.url] = results; + _migrated[node.span.sourceUrl] = results; } - return migrated; + _patches = oldPatches; + } + + /// Visits the stylesheet at [dependency], resolved relative to [source]. + @protected + void visitDependency(Uri dependency, Uri source) { + var stylesheet = parseStylesheet(source.resolveUri(dependency)); + visitStylesheet(stylesheet); } /// Returns the migrated contents of this file, or null if the file does not @@ -78,10 +82,11 @@ abstract class MigrationVisitor extends RecursiveAstVisitor { ? Patch.applyAll(patches.first.selection.file, patches) : null; - /// Resolves [relativeUrl] relative to the current stylesheet's URL. + /// Adds a new patch that should be applied to the current stylesheet. @protected - Uri resolveRelativeUrl(Uri relativeUrl) => - stylesheet.span.file.url.resolveUri(relativeUrl); + void addPatch(Patch patch) { + _patches.add(patch); + } /// If [migrateDependencies] is enabled, any dynamic imports within /// this [node] will be migrated before continuing. @@ -91,7 +96,7 @@ abstract class MigrationVisitor extends RecursiveAstVisitor { if (migrateDependencies) { for (var import in node.imports) { if (import is DynamicImport) { - newInstance(resolveRelativeUrl(Uri.parse(import.url))).run(); + visitDependency(Uri.parse(import.url), node.span.sourceUrl); } } } @@ -103,7 +108,7 @@ abstract class MigrationVisitor extends RecursiveAstVisitor { visitUseRule(UseRule node) { super.visitUseRule(node); if (migrateDependencies) { - newInstance(resolveRelativeUrl(node.url)).run(); + visitDependency(node.url, node.span.sourceUrl); } } } diff --git a/lib/src/migrators/module.dart b/lib/src/migrators/module.dart index 1feeeba9..8eb03630 100644 --- a/lib/src/migrators/module.dart +++ b/lib/src/migrators/module.dart @@ -24,22 +24,13 @@ class ModuleMigrator extends Migrator { final name = "module"; final description = "Migrates stylesheets to the new module system."; - /// Global variables defined at any time during the migrator run. - final _variables = normalizedMap(); - - /// Global mixins defined at any time during the migrator run. - final _mixins = normalizedMap(); - - /// Global functions defined at any time during the migrator run. - final _functions = normalizedMap(); - /// Runs the module migrator on [entrypoint] and its dependencies and returns /// a map of migrated contents. /// /// If [migrateDependencies] is false, the migrator will still be run on /// dependencies, but they will be excluded from the resulting map. Map migrateFile(Uri entrypoint) { - var migrated = _ModuleMigrationVisitor(this, entrypoint).run(); + var migrated = _ModuleMigrationVisitor().run(entrypoint); if (!migrateDependencies) { migrated.removeWhere((url, contents) => url != entrypoint); } @@ -48,48 +39,70 @@ class ModuleMigrator extends Migrator { } class _ModuleMigrationVisitor extends MigrationVisitor { - final ModuleMigrator migrator; + /// Global variables defined at any time during the migrator run. + final _globalVariables = normalizedMap(); - /// Constructs a new module migration visitor. - /// - /// Note: We always set [migratedDependencies] to true since the module - /// migrator needs to always run on dependencies. The `migrateFile` method of - /// the module migrator will filter out the dependencies' migration results. - _ModuleMigrationVisitor(this.migrator, Uri url, {Map migrated}) - : super(url, true, migrated: migrated); + /// Global mixins defined at any time during the migrator run. + final _globalMixins = normalizedMap(); - _ModuleMigrationVisitor newInstance(Uri url) => - _ModuleMigrationVisitor(migrator, url, migrated: migrated); + /// Global functions defined at any time during the migrator run. + final _globalFunctions = normalizedMap(); /// Namespaces of modules used in this stylesheet. - final _namespaces = {}; + Map _namespaces = {}; /// Set of additional use rules necessary for referencing members of /// implicit dependencies / built-in modules. /// /// This set contains the path provided in the use rule, not the canonical /// path (e.g. "a" rather than "dir/a.scss"). - final _additionalUseRules = Set(); + Set _additionalUseRules = Set(); + + /// The URL of the current stylesheet. + Uri _currentUrl; - /// Global variables declared with !default that could be configured. - final _configurableVariables = normalizedSet(); + /// The URL of the last stylesheet that was completely migrated. + Uri _lastUrl; /// Local variables, mixins, and functions for this migration. /// /// When at the top level of the stylesheet, this will be null. LocalScope _localScope; + /// Constructs a new module migration visitor. + /// + /// Note: We always set [migratedDependencies] to true since the module + /// migrator needs to always run on dependencies. The `migrateFile` method of + /// the module migrator will filter out the dependencies' migration results. + _ModuleMigrationVisitor() : super(migrateDependencies: true); + /// Returns the migrated contents of this stylesheet, based on [patches] and /// [_additionalUseRules], or null if the stylesheet does not change. @override String getMigratedContents() { var results = super.getMigratedContents(); if (results == null) return null; - var semicolon = stylesheet.span.file.url.path.endsWith('.sass') ? "" : ";"; + var semicolon = _currentUrl.path.endsWith('.sass') ? "" : ";"; var uses = _additionalUseRules.map((use) => '@use "$use"$semicolon\n'); return uses.join("") + results; } + /// Stores per-file state before visiting [node] and restores it afterwards. + @override + void visitStylesheet(Stylesheet node) { + var oldNamespaces = _namespaces; + var oldAdditionalUseRules = _additionalUseRules; + var oldUrl = _currentUrl; + _namespaces = {}; + _additionalUseRules = Set(); + _currentUrl = node.span.sourceUrl; + super.visitStylesheet(node); + _namespaces = oldNamespaces; + _additionalUseRules = oldAdditionalUseRules; + _lastUrl = _currentUrl; + _currentUrl = oldUrl; + } + /// Visits the children of [node] with a local scope. /// /// Note: The children of a stylesheet are at the root, so we should not add @@ -109,7 +122,7 @@ class _ModuleMigrationVisitor extends MigrationVisitor { @override void visitFunctionExpression(FunctionExpression node) { visitInterpolation(node.name); - patchNamespaceForFunction(node.name.asPlain, (name, namespace) { + _patchNamespaceForFunction(node.name.asPlain, (name, namespace) { addPatch(Patch(node.name.span, "$namespace.$name")); }); visitArgumentInvocation(node.arguments); @@ -124,7 +137,7 @@ class _ModuleMigrationVisitor extends MigrationVisitor { return; } var fnName = nameArgument as StringExpression; - patchNamespaceForFunction(fnName.text.asPlain, (name, namespace) { + _patchNamespaceForFunction(fnName.text.asPlain, (name, namespace) { var span = fnName.span; if (fnName.hasQuotes) { span = span.file.span(span.start.offset + 1, span.end.offset - 1); @@ -144,13 +157,13 @@ class _ModuleMigrationVisitor extends MigrationVisitor { /// when namespaced, and the namespace itself. The name will match the name /// provided to the outer function except for built-in functions whose name /// within a module differs from its original name. - void patchNamespaceForFunction( + void _patchNamespaceForFunction( String name, void patcher(String name, String namespace)) { if (name == null) return; if (_localScope?.isLocalFunction(name) ?? false) return; - var namespace = migrator._functions.containsKey(name) - ? namespaceForNode(migrator._functions[name]) + var namespace = _globalFunctions.containsKey(name) + ? _namespaceForNode(_globalFunctions[name]) : null; if (namespace == null) { @@ -165,7 +178,7 @@ class _ModuleMigrationVisitor extends MigrationVisitor { /// Declares the function within the current scope before visiting it. @override void visitFunctionRule(FunctionRule node) { - declareFunction(node); + _declareFunction(node); super.visitFunctionRule(node); } @@ -187,27 +200,13 @@ class _ModuleMigrationVisitor extends MigrationVisitor { return; } // TODO(jathak): Confirm that this import appears before other rules - var url = resolveRelativeUrl(Uri.parse(import.url)); - var importMigration = newInstance(url)..run(); - _namespaces[importMigration.stylesheet.span.sourceUrl] = - namespaceForPath(import.url); - - var overrides = []; - for (var variable in importMigration._configurableVariables) { - if (migrator._variables.containsKey(variable)) { - var declaration = migrator._variables[variable]; - if (namespaceForNode(declaration) == null) { - overrides.add("\$${declaration.name}: ${declaration.expression}"); - } - // TODO(jathak): Remove this declaration from the current stylesheet if - // it's not referenced before this point. - } - } - var config = ""; - if (overrides.isNotEmpty) { - config = " with (\n " + overrides.join(',\n ') + "\n)"; - } - addPatch(Patch(node.span, '@use ${import.span.text}$config')); + + visitDependency(Uri.parse(import.url), _currentUrl); + _namespaces[_lastUrl] = namespaceForPath(import.url); + + // TODO(jathak): Support configurable variables + + addPatch(Patch(node.span, '@use ${import.span.text}')); } /// Adds a namespace to any mixin include that requires it. @@ -215,8 +214,8 @@ class _ModuleMigrationVisitor extends MigrationVisitor { void visitIncludeRule(IncludeRule node) { super.visitIncludeRule(node); if (_localScope?.isLocalMixin(node.name) ?? false) return; - if (!migrator._mixins.containsKey(node.name)) return; - var namespace = namespaceForNode(migrator._mixins[node.name]); + if (!_globalMixins.containsKey(node.name)) return; + var namespace = _namespaceForNode(_globalMixins[node.name]); if (namespace == null) return; var endName = node.arguments.span.start.offset; var startName = endName - node.name.length; @@ -227,7 +226,7 @@ class _ModuleMigrationVisitor extends MigrationVisitor { /// Declares the mixin within the current scope before visiting it. @override void visitMixinRule(MixinRule node) { - declareMixin(node); + _declareMixin(node); super.visitMixinRule(node); } @@ -244,8 +243,8 @@ class _ModuleMigrationVisitor extends MigrationVisitor { if (_localScope?.isLocalVariable(node.name) ?? false) { return; } - if (!migrator._variables.containsKey(node.name)) return; - var namespace = namespaceForNode(migrator._variables[node.name]); + if (!_globalVariables.containsKey(node.name)) return; + var namespace = _namespaceForNode(_globalVariables[node.name]); if (namespace == null) return; addPatch(Patch(node.span, "\$$namespace.${node.name}")); } @@ -253,22 +252,16 @@ class _ModuleMigrationVisitor extends MigrationVisitor { /// Declares a variable within the current scope before visiting it. @override void visitVariableDeclaration(VariableDeclaration node) { - declareVariable(node); + _declareVariable(node); super.visitVariableDeclaration(node); } /// Declares a variable within this stylesheet, in the current local scope if /// it exists, or as a global variable otherwise. - void declareVariable(VariableDeclaration node) { + void _declareVariable(VariableDeclaration node) { if (_localScope == null || node.isGlobal) { - if (node.isGuarded) { - _configurableVariables.add(node.name); - - // Don't override if variable already exists. - migrator._variables.putIfAbsent(node.name, () => node); - } else { - migrator._variables[node.name] = node; - } + // TODO(jathak): Support configurable variables + _globalVariables[node.name] = node; } else { _localScope.variables.add(node.name); } @@ -276,9 +269,9 @@ class _ModuleMigrationVisitor extends MigrationVisitor { /// Declares a mixin within this stylesheet, in the current local scope if /// it exists, or as a global mixin otherwise. - void declareMixin(MixinRule node) { + void _declareMixin(MixinRule node) { if (_localScope == null) { - migrator._mixins[node.name] = node; + _globalMixins[node.name] = node; } else { _localScope.mixins.add(node.name); } @@ -286,9 +279,9 @@ class _ModuleMigrationVisitor extends MigrationVisitor { /// Declares a function within this stylesheet, in the current local scope if /// it exists, or as a global function otherwise. - void declareFunction(FunctionRule node) { + void _declareFunction(FunctionRule node) { if (_localScope == null) { - migrator._functions[node.name] = node; + _globalFunctions[node.name] = node; } else { _localScope.functions.add(node.name); } @@ -296,12 +289,12 @@ class _ModuleMigrationVisitor extends MigrationVisitor { /// Finds the namespace for the stylesheet containing [node], adding a new use /// rule if necessary. - String namespaceForNode(SassNode node) { - if (node.span.sourceUrl == stylesheet.span.sourceUrl) return null; + String _namespaceForNode(SassNode node) { + if (node.span.sourceUrl == _currentUrl) return null; if (!_namespaces.containsKey(node.span.sourceUrl)) { /// Add new use rule for indirect dependency var relativePath = p.relative(node.span.sourceUrl.path, - from: p.dirname(stylesheet.span.sourceUrl.path)); + from: p.dirname(_currentUrl.path)); var basename = p.basenameWithoutExtension(relativePath); if (basename.startsWith('_')) basename = basename.substring(1); var simplePath = p.relative(p.join(p.dirname(relativePath), basename)); diff --git a/test/migrator_utils.dart b/test/migrator_utils.dart index 5a2b13a5..4f11ba3f 100644 --- a/test/migrator_utils.dart +++ b/test/migrator_utils.dart @@ -49,9 +49,10 @@ _testHrx(File hrxFile, String migrator) async { migrated = await MigratorRunner().run(arguments); }, getCurrentDirectory: () => Directory(d.sandbox)), prints(files.expectedLog?.replaceAll("\$TEST_DIR", d.sandbox) ?? "")); - var canonicalize = FilesystemImporter(d.sandbox).canonicalize; + var importer = FilesystemImporter(d.sandbox); for (var file in files.input.keys) { - expect(migrated[canonicalize(Uri.parse(file))], equals(files.output[file]), + expect(migrated[importer.canonicalize(Uri.parse(file))], + equals(files.output[file]), reason: 'Incorrect migration of $file.'); } } diff --git a/test/migrators/module/module_configuration.hrx b/test/migrators/module/module_configuration.hrx deleted file mode 100644 index c7fe9ac8..00000000 --- a/test/migrators/module/module_configuration.hrx +++ /dev/null @@ -1,18 +0,0 @@ -<==> arguments ---migrate-deps - -<==> input/entrypoint.scss -$config: red; -$no-config: blue; -@import "library"; - -<==> input/_library.scss -$config: green !default; -$no-config: yellow; - -<==> output/entrypoint.scss -$config: red; -$no-config: blue; -@use "library" with ( - $config: red -); From 5ac0dc609b8c0fa57fd0371e423c82f92e59591d Mon Sep 17 00:00:00 2001 From: Jennifer Thakar Date: Wed, 17 Apr 2019 17:26:29 -0700 Subject: [PATCH 7/7] Fix nits --- lib/src/migrators/module.dart | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/lib/src/migrators/module.dart b/lib/src/migrators/module.dart index 8eb03630..386f5e35 100644 --- a/lib/src/migrators/module.dart +++ b/lib/src/migrators/module.dart @@ -49,14 +49,14 @@ class _ModuleMigrationVisitor extends MigrationVisitor { final _globalFunctions = normalizedMap(); /// Namespaces of modules used in this stylesheet. - Map _namespaces = {}; + Map _namespaces; /// Set of additional use rules necessary for referencing members of /// implicit dependencies / built-in modules. /// /// This set contains the path provided in the use rule, not the canonical /// path (e.g. "a" rather than "dir/a.scss"). - Set _additionalUseRules = Set(); + Set _additionalUseRules; /// The URL of the current stylesheet. Uri _currentUrl; @@ -84,7 +84,7 @@ class _ModuleMigrationVisitor extends MigrationVisitor { if (results == null) return null; var semicolon = _currentUrl.path.endsWith('.sass') ? "" : ";"; var uses = _additionalUseRules.map((use) => '@use "$use"$semicolon\n'); - return uses.join("") + results; + return uses.join() + results; } /// Stores per-file state before visiting [node] and restores it afterwards. @@ -118,7 +118,7 @@ class _ModuleMigrationVisitor extends MigrationVisitor { _localScope = _localScope.parent; } - /// Adds a namespace to any function call that require it. + /// Adds a namespace to any function call that requires it. @override void visitFunctionExpression(FunctionExpression node) { visitInterpolation(node.name);