Use latest Dart Sass and release migrator 2.0.0 #251

jathak · 2024-01-03T04:33:18Z

Deleted the obsolete media-logic migrator (breaking change bumps version to 2.0.0).
Moved off of the tuple package to native Dart tuples.
Fixed the calc-interpolation and division migrators to eliminate their use of the removed CalculationExpression AST node.
Created a new ScopedAstVisitor that uses Scope to track Sass member declarations. MigrationVisitor and _ReferenceVisitor now extend this.
Refactored the division migrator to use patterns.

- Deleted the obsolete `media-logic` migrator (breaking change bumps version to 2.0.0). - Moved off of the tuple package to native Dart tuples. - Fixed the `calc-interpolation` and `division` migrators to eliminate their use of the removed `CalculationExpression` AST node. - Created a new `ScopedAstVisitor` that uses `Scope` to track Sass member declarations. `MigrationVisitor` now extends this. - Refactored the `division` migrator to use patterns.

nex3 · 2024-01-03T23:12:25Z

lib/src/migrators/division.dart

-      return false;
-    }
+    var channels = switch (node.arguments) {
+      ArgumentInvocation(positional: [ListExpression arg, ...], named: Map()) =>


Should this be [ListExpression arg]? The old code only matches if there's only one positional argument.

Also, I think named: Map() just checks that named's type is Map. If you want to check that it's empty, you'll need to do named: Map(length: 0).

nex3 · 2024-01-03T23:15:10Z

lib/src/migrators/division.dart

+            left: NumberExpression _,
+            right: NumberExpression _


Here I think NumberExpression() is more idiomatic

nex3 · 2024-01-03T23:18:13Z

lib/src/migrators/division.dart

+      switch (last) {
+        case BinaryOperationExpression(
+            left: NumberExpression _,
+            right: NumberExpression _
+          ):
+          break;
+        default:
+          _patchSpacesToCommas(channels);
+          _patchOperatorToComma(last);
+      }


This could just be:

Suggested change

switch (last) {

case BinaryOperationExpression(

left: NumberExpression _,

right: NumberExpression _

):

break;

default:

_patchSpacesToCommas(channels);

_patchOperatorToComma(last);

}

if (last case BinaryOperationExpression(

left: NumberExpression(),

right: NumberExpression()

)) {

break;

}

_patchSpacesToCommas(channels);

_patchOperatorToComma(last);

The break was to break out of the switch, since we only want to call the _patch* functions when the pattern is not matched, but we do still want to do the _withContext part either way. Is this structure (with an empty if and the patch calls in an else) better than the switch?

Oh, I see. In Dart 3 you no longer need a break at the end of a case block; they don't fall through unless either the block is empty or you explicitly opt into it with continue.

In this case I'd like if/case/else.

What is the idiomatic way to have an empty case block that doesn't fall through?

In that case you need break, but only that case.

nex3 · 2024-01-03T23:19:18Z

lib/src/migrators/division.dart

-    addPatch(Patch(node.right.span, '${1 / divisor.value}'));
-    return true;
+    if (node.right case NumberExpression(unit: null, value: var divisor)) {
+      if (!_allowedDivisors.contains(divisor)) return false;


I'd put this in a when clause since it's logically part of the same condition as the if.

Done. Forgot that when clauses where an option.

nex3 · 2024-01-03T23:20:15Z

lib/src/migrators/division.dart

-      return node.operator == BinaryOperator.dividedBy &&
-          _onlySlash(node.left) &&
-          _onlySlash(node.right);
+    switch (node) {


I'd make this a switch expression

nex3 · 2024-01-03T23:27:52Z

lib/src/migrators/module.dart

      var tuple = importCache.canonicalize(ruleUrl,
          baseImporter: importer, baseUrl: currentUrl, forImport: true);


Destructure this tuple

nex3 · 2024-01-03T23:30:15Z

lib/src/migrators/module.dart

    var tuple = _originalImports[url];
-    if (tuple != null && tuple.item2 is NodeModulesImporter) {
-      return Tuple2(tuple.item1, false);
+    if (tuple case (var url, NodeModulesImporter _)) {


Nit: I'd inline the variable here.

nex3 · 2024-01-03T23:31:36Z

lib/src/util/scoped_ast_visitor.dart

+/// current scope.
+abstract class ScopedAstVisitor
+    with RecursiveStatementVisitor, RecursiveAstVisitor {
+  /// The current scope, containing any visible without a namespace to the


Any visible what?

nex3 · 2024-01-03T23:33:07Z

lib/src/util/scoped_ast_visitor.dart

+        var defaultValue = argument.defaultValue;
+        if (defaultValue != null) visitExpression(defaultValue);


Nit: Prefer if/case for situations where a variable is only used within a null-check block, so the variable's scope matches its usage

nex3 · 2024-01-03T23:34:34Z

lib/src/migration_visitor.dart


      var oldImporter = _importer;
-      _importer = result.item1;
-      var stylesheet = result.item2;
+      var oldScope = currentScope;


Consider adding a when: parameter to scoped() and using that here.

scoped() adds a new child scope, whereas here we're creating a new empty scope for a new module.

jathak added 2 commits January 3, 2024 02:51

Have _ReferenceVisitor use ScopedAstVisitor

a782328

jathak requested a review from nex3 January 3, 2024 04:33

nex3 requested changes Jan 3, 2024

View reviewed changes

Code review

63280b3

jathak requested a review from nex3 January 4, 2024 00:57

nex3 approved these changes Jan 4, 2024

View reviewed changes

Final code review

1ad0142

jathak merged commit 387292f into main Jan 4, 2024
17 checks passed

jathak deleted the support-latest-sass branch January 4, 2024 22:55

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use latest Dart Sass and release migrator 2.0.0 #251

Use latest Dart Sass and release migrator 2.0.0 #251

jathak commented Jan 3, 2024

nex3 Jan 3, 2024

jathak Jan 4, 2024

nex3 Jan 3, 2024

jathak Jan 4, 2024

nex3 Jan 3, 2024

jathak Jan 4, 2024

nex3 Jan 4, 2024

jathak Jan 4, 2024

nex3 Jan 5, 2024

nex3 Jan 3, 2024

jathak Jan 4, 2024

nex3 Jan 3, 2024

jathak Jan 4, 2024

nex3 Jan 3, 2024

jathak Jan 4, 2024

nex3 Jan 3, 2024

jathak Jan 3, 2024

nex3 Jan 3, 2024

jathak Jan 3, 2024

nex3 Jan 3, 2024

jathak Jan 3, 2024

nex3 Jan 3, 2024

jathak Jan 3, 2024

		var tuple = importCache.canonicalize(ruleUrl,
		baseImporter: importer, baseUrl: currentUrl, forImport: true);

		var defaultValue = argument.defaultValue;
		if (defaultValue != null) visitExpression(defaultValue);

Use latest Dart Sass and release migrator 2.0.0 #251

Use latest Dart Sass and release migrator 2.0.0 #251

Conversation

jathak commented Jan 3, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment