Support import-only files with include rules by jathak · Pull Request #300 · sass/migrator

jathak · 2026-05-20T18:26:10Z

This enables migration of some nested/late imports that the migrator couldn't previously handle by first wrapping the file that's depended on via nested import in a mixin and then @includeing that mixin in the import-only file.

jathak · 2026-05-20T19:14:23Z

#301 should fix the CI here

This enables migration of some nested/late imports that the migrator couldn't previously handle by first wrapping the file that's depended on via nested import in a mixin and then `@include`ing that mixin in the import-only file.

nex3 · 2026-05-21T23:47:22Z

The tests that are exercising nested imports should probably mention that in their titles.

nex3 · 2026-05-21T23:57:37Z

+        throw MigrationSourceSpanException(
+            "Found a second @include rule in an import-only file. "
+            "Import-only files should contain at most one @include.",
+            node.span);


Would it be possible to support multiple rules if we stored _importOnlyIncludes' values as lists?

Yeah, I initially avoided it because I thought it would be complicated turning one @import rule into multiple @include rules, but I forgot we already had logic for turning one rule into multiple because an @import rule can contain multiple imports. Done.

nex3 · 2026-05-21T23:59:01Z

+            '${_namespaceForDeclaration(references.variables[arg]!)}.\$$name',
+          VariableExpression(:var name) => '\$$name',
+          _ => throw MigrationSourceSpanException(
+              'Import-only @include rules may only pass variables as arguments.',


Why this restriction? It seems like at the very least it wouldn't hurt to allow literal values, and really the only semantically difficult argument I can think of would be user-defined functions (which function more or less like variables anyway).

I initially restricted it to avoid having to recursively serialize all expressions (making sure to add any additional @use rules as necessary), but I ended up writing a serializer that can add/change namespaces as necessary, so that eliminates the need for this restriction.

This adds a `NamespacedSerializer` utility class, which allows us to copy the include rules with arbitrary arguments while still ensuring that we use the right namespaces and add any additional use rules when migrating.

jathak

Added NamespacedSerializer utility class to allow for arbitrary arguments to be copied from the @include rule in the import-only file. It does copy a fair bit of logic from toString() methods on some of the AST nodes, so let me know if there's a better way to implement this to avoid the duplicated code.

jathak · 2026-05-22T14:50:55Z

+        throw MigrationSourceSpanException(
+            "Found a second @include rule in an import-only file. "
+            "Import-only files should contain at most one @include.",
+            node.span);


Yeah, I initially avoided it because I thought it would be complicated turning one @import rule into multiple @include rules, but I forgot we already had logic for turning one rule into multiple because an @import rule can contain multiple imports. Done.

jathak · 2026-05-22T18:33:08Z

+            '${_namespaceForDeclaration(references.variables[arg]!)}.\$$name',
+          VariableExpression(:var name) => '\$$name',
+          _ => throw MigrationSourceSpanException(
+              'Import-only @include rules may only pass variables as arguments.',


I initially restricted it to avoid having to recursively serialize all expressions (making sure to add any additional @use rules as necessary), but I ended up writing a serializer that can add/change namespaces as necessary, so that eliminates the need for this restriction.

jathak · 2026-05-22T18:33:47Z

nex3 · 2026-06-02T02:47:37Z

Consider publicly exposing a function (or several functions) from this file rather than a class, since in practice the class is only ever used to call a single method and then discarded.

nex3 · 2026-06-02T02:51:40Z

+/// A utility for serializing Sass expressions while adding or changing
+/// namespaces of Sass references.


This means that the migrator now effectively has two different ways of modifying an expression: one by patching, and now this which serializes from scratch. Is it possible to consolidate these back down into a patch? For example, could you represent this as one patch that adds the original text of the @include from the import-only file, followed by additional patches that adjust its namespaces (possibly using something like Dart Sass's interpolation maps to realign spans from the original import-only file to the new @include location)? My two practical concerns here are:

This is duplicating a lot of the Sass inspect/serialization logic, which means more work you'll have to do any time the Sass AST changes.

This throws away any formatting/comments/etc from the original source.

If you think this distinction makes more sense than trying to make it all work via patching, I do think it's probably worth including in the documentation some discussion of when this is appropriate relative to the original patch-based approach.

As is, the migrator won't allow patches like this, since the patch offsets are always based on the original file, so after patching in the original text, it's not possible to add additional patches within it, but I do agree that the patch-based approach makes more sense if at all possible, so I'll look into adding something that can do the inverse: first patch the span of the original @include (without actually patching the underlying source file) and then apply that patch that patched span into the import-only file.

nex3 · 2026-06-02T03:00:43Z

+                [for (var item in contents) serialize(item)].join(
+                  switch (separator) {
+                    .comma => ', ',
+                    .space => ' ',


Suggested change

.space => ' ',

.space || .undecided => ' ',

nex3 · 2026-06-02T03:03:44Z

As before, since this is testing nested includes it should mention that in its title.

jathak

Updated the logic here to use patching instead of serialization.

jathak · 2026-06-03T17:12:50Z

jathak · 2026-06-03T17:13:15Z

jathak requested a review from nex3 May 20, 2026 18:26

Support import-only files with include rules

c2f4ecf

This enables migration of some nested/late imports that the migrator couldn't previously handle by first wrapping the file that's depended on via nested import in a mixin and then `@include`ing that mixin in the import-only file.

jathak force-pushed the import-only-includes branch from 094755e to c2f4ecf Compare May 21, 2026 13:56

nex3 reviewed May 21, 2026

View reviewed changes

jathak added 2 commits May 22, 2026 11:33

Allow multiple includes and arbitrary arguments

2079383

This adds a `NamespacedSerializer` utility class, which allows us to copy the include rules with arbitrary arguments while still ensuring that we use the right namespaces and add any additional use rules when migrating.

Run formatter

97af593

jathak commented May 22, 2026

View reviewed changes

jathak requested a review from nex3 May 27, 2026 15:19

nex3 requested changes Jun 2, 2026

View reviewed changes

Use patching instead of serialization

2fd109c

jathak commented Jun 3, 2026

View reviewed changes

jathak requested a review from nex3 June 3, 2026 17:15

nex3 approved these changes Jun 8, 2026

View reviewed changes

jathak merged commit 045e956 into main Jun 8, 2026
18 checks passed

jathak deleted the import-only-includes branch June 8, 2026 21:02

		/// A utility for serializing Sass expressions while adding or changing
		/// namespaces of Sass references.

Uh oh!

Conversation

jathak commented May 20, 2026

Uh oh!

jathak commented May 20, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jathak left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jathak left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants