feat: Rework getting plugin implementation candidates and plugin resolution #145258
Conversation
github-actions
bot
added
the
tool
Gustl22
force-pushed
the
80374-refactor-5
branch
from
ab1a837 to
4bd60c1
Compare
Gustl22
force-pushed
the
80374-refactor-5
branch
from
4bd60c1 to
b5d72e4
Compare
| WindowsPlugin.kConfigKey, | ||
| ]; | ||
| final List<PluginInterfaceResolution> finalResolution = <PluginInterfaceResolution>[]; | ||
| final List<PluginInterfaceResolution> pluginResolutions = <PluginInterfaceResolution>[]; |
There was a problem hiding this comment.
Adapted the naming, as "final" is not very descriptive. Instead renaming the other scoped variables should add more clarity.
| for (final String platformKey in platformKeys) { | ||
| // Key: the plugin name | ||
| final Map<String, List<Plugin>> possibleResolutions = <String, List<Plugin>>{}; | ||
| final Map<String, List<Plugin>> pluginImplCandidates = <String, List<Plugin>>{}; |
There was a problem hiding this comment.
Speaking now of "implementation candidates" as used here already.
| String? implementsPackage = plugin.implementsPackage; | ||
| if (implementsPackage == null || implementsPackage.isEmpty) { | ||
| final bool hasInlineDartImplementation = | ||
| plugin.pluginDartClassPlatforms[platformKey] != null; |
There was a problem hiding this comment.
|
It looks like this pull request may not have tests. Please make sure to add tests before merging. If you need an exemption to this rule, contact "@test-exemption-reviewer" in the #hackers channel in Chat (don't just cc them here, they won't see it! Use Discord!). If you are not sure if you need tests, consider this rule of thumb: the purpose of a test is to make sure someone doesn't accidentally revert the fix. Ask yourself, is there anything in your PR that you feel it is important we not accidentally revert back to how it was before your fix? Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing. |
|
\cc @christopherfujino @stuartmorgan for sneak in a review / adding reviewers. Needs adding test-exempt as its a refactor only. Thanks again for taking the time. |
|
test-exempt: code refactor with no semantic change |
|
Thanks for the very fast review 🎉! I try to resolve your comments in the next few days :) |
|
Thank you very much for the fast and helpful feedback. The method description was admittedly misleading. |
|
@stuartmorgan any chance to throw in a review before the weekend. Merci :) |
|
@stuartmorgan If you have time, can you give me a quick feedback / clarification to my answers, so I'll try to implement them accordingly. |
Gustl22
force-pushed
the
80374-refactor-5
branch
2 times, most recently
from
13656ab to
918ca5d
Compare
Gustl22
changed the title
stuartmorgan-g
left a comment
stuartmorgan-g
left a comment
There was a problem hiding this comment.
Thanks, I find this version far easier to follow! Just small things left.
|
|
||
| if (defaultImplPluginName != null) { | ||
| if (implementsPackage != null && implementsPackage.isNotEmpty) { | ||
| throwToolExit( |
There was a problem hiding this comment.
Why do we need this new error? It would certainly be a deeply weird structure for a plugin to be both an implementation of another plugin and also a platform interface for itself, but why do we need to go to the extent of forbidding it?
There was a problem hiding this comment.
For explaining that, we have to look into the former implementation. There, the case was not really considered. It was assumed, that if a package has an implementsPackage, it does not also have a defaultImplPlugin, so it was registered as implementation only, but did not reference a default plugin, which is wrong (unless I missed something).
So we can allow this of course, but then I would also add / rewrite that test to support that behavior officially.
Then we also have to evaluate, if the defaultImplPlugin serves also as default implementation for implementsPackage (the app-facing one) or just for the current handled plugin (in the middle of the dependency tree). So consequently the current plugin then must not serve as implementation for the app-facing package, but it's default implementation should have it's role, which brings back the complicated relations between defaultImpl and implements in the code.
The only scenario, I can think of is, that a federated plugin implements multiple platforms (inline, e.g. for ios, macos). But uses a default implementation for e.g. android, which was not added as default implementation in the app-facing package, for whatever reason.
But this behavior wasn't supported until now (at least how I interpret the current code), and this needs precise testing (probably with a real world example, if the changes actually work), in case we want to support it.
There was a problem hiding this comment.
I would propose opening an issue and link it in the error description, so one with a valid use case can further contribute to this. I think this was not explicitly part of the design doc and I would avoid enabling more features, which are beyond the scope of this PR.
There was a problem hiding this comment.
Ideally we would preserve the existing behavior rather than introduce a new hard error, but on the other hand since currently anything doing this is wrong, I guess I'm fine with a new error. If we get issues filed because some packages are (presumably accidentally) actually doing this, we can re-evaluate.
There was a problem hiding this comment.
I changed the error description to be more engaging, so the devs know, that this is not a hard limitation (685285f)
|
@stuartmorgan Thanks for the review! With exception of the error for disallowing default/implements at the same time, I tackled all the proposals. Let me know, what you think, how we should proceed in this case. Best regards. |
christopherfujino
added
the
autosubmit
|
Thank you both for taking the time! |
3 participants
Part of #137040 and #80374
Pre-launch Checklist
///).If you need help, consider asking for advice on the #hackers-new channel on Discord.