Use package:_macro_host in _analyzer_macros. #28
Conversation
davidmorgan
force-pushed
the
end-to-end
branch
2 times, most recently
from
7d9bb52
to
175c819
Compare
| import 'package:macros/src/executor.dart' as injected; | ||
|
|
||
| /// Injected macro implementation for the analyzer. | ||
| class MacroImplementation implements injected.MacroImplementation, HostService { |
There was a problem hiding this comment.
On the flip side here, I would call this AnalyzerMacroImplementation, to avoid duplicate names. Same for all those below.
| /// Injected macro implementation for the analyzer. | ||
| class MacroImplementation implements injected.MacroImplementation, HostService { | ||
| final Uri packageConfig; | ||
| final Map<String, String> macroImplByName; |
There was a problem hiding this comment.
I can infer the keys are names (probably class names), but I don't know what the values are. Can you add a comment describing this a bit?
There was a problem hiding this comment.
It's described in the comment where it's passed in to start: value are fully qualified name of the implementation.
This will anyway go away in the next PR when the placeholder pubspec reader is used :)
| required Map<String, String> macroImplByName, | ||
| }) async { | ||
| final result = MacroImplementation._(packageConfig, macroImplByName); | ||
| result._host = await MacroHost.serve( |
There was a problem hiding this comment.
I see this pattern is again repeated a bunch across these implementations. I will stop harping on it but something about the design here definitely seems off, that we require so much late initialization and cyclic dependencies.
There was a problem hiding this comment.
It's probably always correct to refactor, so please do keep mentioning :) thanks.
Here again it's due to the class doing two things; moved the host service to a separate class.
| } | ||
|
|
||
| class MacroPackageConfigs implements injected.MacroPackageConfigs { | ||
| final MacroImplementation impl; |
| MacroPackageConfigs(this.impl); | ||
|
|
||
| @override | ||
| bool isMacro(Uri uri, String name) => |
There was a problem hiding this comment.
Seems a bit weird that this is just a wrapper which calls a function by the same name, but which takes slightly different args. Can we unify the signatures? Similar patterns below
There was a problem hiding this comment.
That's because the injected API assumes the package config URI is known from context, but the _macro_host API wants it passed in.
We could update _macro_host API to match, also assuming it from context, but it's not clear to me yet what the scope of one MacroHost will be, it only makes sense if it's going to be for a single package config.
So, maybe :)
|
|
||
| RunningMacro._(this.impl, this.name, this.implementation); | ||
|
|
||
| static RunningMacro run(MacroImplementation impl, QualifiedName name, |
There was a problem hiding this comment.
Can this instead return a Future<RunningMacro> and we can drop the _started field and the awaits below?
There was a problem hiding this comment.
Yeeeah that's what I had originally, but the RunningMacro method that gets injected in the analyzer is sync so this has to be sync. I added a TODO to consider changing that upstream :)
| } | ||
| } | ||
|
|
||
| /// Converts [AugmentResponse] to [injected.MacroExecutionResult]. |
There was a problem hiding this comment.
Should we have TODOs for the empty responses here?
There was a problem hiding this comment.
This class is basically one big TODO :) added at the top.
This now actually builds and runs the test macro via a host injected into the analyer.