Warn instead of crashing for bad URLs #63
Conversation
There was a problem hiding this comment.
I'm concerned that this behavior, printing a warning for each unresolvable import, doesn't work well for some likely use-cases. Let's think through some scenarios where a user may have unresolvable imports:
-
A user has a typo in a single import. The current behavior works well here: it shows them exactly where the typo is and what's wrong with it. This is also pretty unlikely, though, since the Sass compiler also won't be able to run on this code.
-
A user is running a non-dependency-aware migrator like
divisionand only cares about migrating their own stylesheets. Even once Gracefully handle dependencies from load paths and importers #58 is fixed, they won't want to pass load paths along with-dbecause they don't own their dependencies. Warnings about unresolvable imports are pure annoyance to them. -
A user is running a dependency-aware migrator like
module. Whether or not they want to migrate their dependencies, the migrator needs to be able to load them to perform the migration. Printing a warning here is also wrong; it should be an error message.
Code-wise, I think this points to putting each each MigrationVisitor in charge of what to do if an import fails to load. The default visitor could aggregate all failed imports and print a single warning after the rest of the migration is done, to avoid cluttering the output for use case 2, and _ModuleMigrationVisitor could somehow override it to emit an error instead.
|
Okay, I changed the default to combine all warnings into one (when The module migrator now throws a MigrationException, which the runner now catches and prints. |
lib/src/migration_visitor.dart
Outdated
| @@ -34,6 +35,11 @@ abstract class MigrationVisitor extends RecursiveAstVisitor { | |||
| /// True if dependencies should be migrated as well. | |||
| final bool migrateDependencies; | |||
|
|
|||
| /// Map of missing dependency URLs to the spans that import/use them. | |||
| UnmodifiableMapView<Uri, FileSpan> get missingDependencies => | |||
There was a problem hiding this comment.
| UnmodifiableMapView<Uri, FileSpan> get missingDependencies => | |
| Map<Uri, FileSpan> get missingDependencies => |
lib/src/migration_visitor.dart
Outdated
| if (stylesheet != null) { | ||
| visitStylesheet(stylesheet); | ||
| } else { | ||
| _missingDependencies[url] = context; |
There was a problem hiding this comment.
| _missingDependencies[url] = context; | |
| _missingDependencies.putIfAbsent(url, () => context); |
This is minor, but I think it's more likely that an earlier import is more relevant to the user.
| @@ -26,6 +28,9 @@ abstract class Migrator extends Command<Map<Uri, String>> { | |||
| /// If true, dependencies will be migrated in addition to the entrypoints. | |||
| bool get migrateDependencies => globalResults['migrate-deps'] as bool; | |||
|
|
|||
| /// Map of missing dependency URLs to the spans that import/use them. | |||
| final missingDependencies = <Uri, FileSpan>{}; | |||
There was a problem hiding this comment.
Document how subclasses are intended to interact with this.
| var visitor = | ||
| _DivisionMigrationVisitor(this.isPessimistic, migrateDependencies); | ||
| var result = visitor.run(entrypoint); | ||
| missingDependencies.addAll(visitor.missingDependencies); |
There was a problem hiding this comment.
Bleh, it sucks that each subclass has to do this manually rather than just having "warn for missing dependencies" be the automatic default behavior. It's not something that needs to happen in this PR, but it's worth thinking about how you could make this more automatic.
There was a problem hiding this comment.
Yeah, that's why I originally passed in the list for the visitor to add to, but this is probably cleaner than that, particularly right now when there's only one migrator that actually uses missingDependencies. Created #64 for this.
Fixes #57. Fixes #59.
The migrator will now print a warning for any URL that doesn't exist (both entrypoints and dependencies).