Support import-only files with include rules#300
Conversation
|
#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.
jathak
force-pushed
the
import-only-includes
branch
from
094755e to
c2f4ecf
Compare
There was a problem hiding this comment.
The tests that are exercising nested imports should probably mention that in their titles.
| throw MigrationSourceSpanException( | ||
| "Found a second @include rule in an import-only file. " | ||
| "Import-only files should contain at most one @include.", | ||
| node.span); |
There was a problem hiding this comment.
Would it be possible to support multiple rules if we stored _importOnlyIncludes' values as lists?
There was a problem hiding this comment.
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.
| '${_namespaceForDeclaration(references.variables[arg]!)}.\$$name', | ||
| VariableExpression(:var name) => '\$$name', | ||
| _ => throw MigrationSourceSpanException( | ||
| 'Import-only @include rules may only pass variables as arguments.', |
There was a problem hiding this comment.
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).
There was a problem hiding this comment.
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
left a comment
jathak
left a comment
There was a problem hiding this comment.
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.
| throw MigrationSourceSpanException( | ||
| "Found a second @include rule in an import-only file. " | ||
| "Import-only files should contain at most one @include.", | ||
| node.span); |
There was a problem hiding this comment.
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.
| '${_namespaceForDeclaration(references.variables[arg]!)}.\$$name', | ||
| VariableExpression(:var name) => '\$$name', | ||
| _ => throw MigrationSourceSpanException( | ||
| 'Import-only @include rules may only pass variables as arguments.', |
There was a problem hiding this comment.
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.
2 participants
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.