sass / migrator

Tool for migrating stylesheets to new Sass versions
MIT License
84 stars 10 forks source link

Migration crashes when migrating multiple stylesheets with a !default declaration of the same name #112

Closed esteinborn closed 4 years ago

esteinborn commented 4 years ago

When running with --migrate-deps option I receive an error message. Running without --migrate-deps successfully converts my style.scss without any issue.

sass-migrator --migrate-deps module --forward=all docroot/themes/custom/mytheme/scss/style.scss
/Users/eric/.nvm/versions/node/v10.15.3/lib/node_modules/sass-migrator/sass_migrator.dart.js:5870
throw u}}
^

TypeError: Cannot read property 'M' of null
    at fD.m5 (/Users/eric/.nvm/versions/node/v10.15.3/lib/node_modules/sass-migrator/sass_migrator.dart.js:13301:4)
    at fD.cB (/Users/eric/.nvm/versions/node/v10.15.3/lib/node_modules/sass-migrator/sass_migrator.dart.js:13261:29)
    at eR.q (/Users/eric/.nvm/versions/node/v10.15.3/lib/node_modules/sass-migrator/sass_migrator.dart.js:8339:24)
    at eR.w (/Users/eric/.nvm/versions/node/v10.15.3/lib/node_modules/sass-migrator/sass_migrator.dart.js:8340:27)
    at fD.aU (/Users/eric/.nvm/versions/node/v10.15.3/lib/node_modules/sass-migrator/sass_migrator.dart.js:12562:39)
    at fD.aU (/Users/eric/.nvm/versions/node/v10.15.3/lib/node_modules/sass-migrator/sass_migrator.dart.js:13189:3)
    at fD.bh (/Users/eric/.nvm/versions/node/v10.15.3/lib/node_modules/sass-migrator/sass_migrator.dart.js:12536:28)
    at fD.bh (/Users/eric/.nvm/versions/node/v10.15.3/lib/node_modules/sass-migrator/sass_migrator.dart.js:12793:3)
    at fD.bh (/Users/eric/.nvm/versions/node/v10.15.3/lib/node_modules/sass-migrator/sass_migrator.dart.js:13141:3)
    at fD.kh (/Users/eric/.nvm/versions/node/v10.15.3/lib/node_modules/sass-migrator/sass_migrator.dart.js:12788:6)
jathak commented 4 years ago

Can you provide a link to the stylesheets that you're attempting to migrate so I can attempt to reproduce?

If you can't do that, could you try running the Dart version (installed through the standalone release or through a package manager other than Node) and providing the stack trace you get from it so I can attempt to debug?

esteinborn commented 4 years ago

Here is a shortened Gist of one of the file combinations causing an error. https://gist.github.com/esteinborn/6642936a3254ee3badd60f6467b2c47f

It would seem that this might be occurring due to using !default in both the _default-variables.scss and _tables.scss files for variables.

esteinborn commented 4 years ago

Running using the standalone dart version this is the error I get:

 sass-migrator --verbose --migrate-deps module --forward=all docroot/themes/custom/mytheme/scss/style.scss
Unhandled exception:
NoSuchMethodError: The method 'add' was called on null.
Receiver: null
Tried calling: add(Instance of 'MemberDeclaration<VariableDeclaration>')
#0      Object.noSuchMethod (dart:core-patch/object_patch.dart:51:5)
#1      _ModuleMigrationVisitor._migrateImport (package:sass_migrator/src/migrators/module.dart:729:32)
#2      _ModuleMigrationVisitor.visitImportRule (package:sass_migrator/src/migrators/module.dart:650:7)
#3      ImportRule.accept (package:sass/src/ast/sass/statement/import_rule.dart:21:55)
#4      RecursiveStatementVisitor.visitChildren (package:sass/src/visitor/recursive_statement.dart:217:13)
#5      _ModuleMigrationVisitor.visitChildren (package:sass_migrator/src/migrators/module.dart:493:11)
#6      RecursiveStatementVisitor.visitStylesheet (package:sass/src/visitor/recursive_statement.dart:135:41)
#7      MigrationVisitor.visitStylesheet (package:sass_migrator/src/migration_visitor.dart:91:11)
#8      _ModuleMigrationVisitor.visitStylesheet (package:sass_migrator/src/migrators/module.dart:358:11)
#9      MigrationVisitor.run (package:sass_migrator/src/migration_visitor.dart:75:5)
#10     _ModuleMigrationVisitor.run (package:sass_migrator/src/migrators/module.dart:196:26)
#11     ModuleMigrator.migrateFile (package:sass_migrator/src/migrators/module.dart:100:28)
#12     Migrator.run (package:sass_migrator/src/migrator.dart:85:22)
#13     ModuleMigrator.run (package:sass_migrator/src/migrators/module.dart:68:25)
#14     CommandRunner.runCommand (package:args/command_runner.dart:197:27)
<asynchronous suspension>
#15     MigratorRunner.execute (package:sass_migrator/src/runner.dart:81:24)
<asynchronous suspension>
#16     main (file:///home/travis/build/sass/migrator/bin/sass_migrator.dart:16:20)
#17     _startIsolate.<anonymous closure> (dart:isolate-patch/isolate_patch.dart:303:32)
#18     _RawReceivePortImpl._handleMessage (dart:isolate-patch/isolate_patch.dart:172:12)
jathak commented 4 years ago

Okay, so it looks like the issue here is that the migrator treats the declaration in _default-variables.scss as configuring the declaration in _tables.scss, but since _default-variables.scss doesn't depend on _tables.scss, it doesn't migrate it to a @use rule configuration and it tries to add it to the set of configurable variables for an upstream dependency that doesn't exist.

I think we can fix this by only considering a variable "externally configured" if it was overridden in a stylesheet that's upstream from the default declaration, and not just loaded first.

esteinborn commented 4 years ago

Wow, quick work!

How can I assist in testing this further? I'm using the release from 7 days ago as standalone release.