-
Notifications
You must be signed in to change notification settings - Fork 1.7k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Shell routing and Service registration automation #13402
Shell routing and Service registration automation #13402
Conversation
Hey there @egvijayanand! Thank you so much for your PR! Someone from the team will get assigned to your PR shortly and we'll get it reviewed. |
Thank you for your pull request. We are auto-formating your source code to follow our code guidelines. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking much better, thanks a lot for the effort. I think we can improve the code a little more taking into account that some else clauses are not needed and that we can switch on a string?
@dotnet-policy-service agree |
@egvijayanand FYI, @PureWeen opened a discussion around this feature on MCT repo Not sure if this will be approved here or there, but in any case, you should use |
Even I've no clarity on whether this would be part of .NET MAUI / Toolkit. Still, reviews are being done. Will check on the Incremental part of SG. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not my area of expertise so we should have another review (I'll ping the right person). Love the code changes, code is much nicer to read THANKS a lot for the hard work.
I left two nits, definitely not blockers but nice to have for nullability.
} | ||
|
||
var addComment = true; | ||
SymbolInfo symbolInfo; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: (and I'm not sure)
SymbolInfo symbolInfo; | |
SymbolInfo? symbolInfo; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since the return value is non-nullable from the Semantic Model of the SG, didn't consider it as marking as nullable.
{ | ||
IdentifierNameSyntax identifierName => identifierName.Identifier.ValueText, | ||
MemberAccessExpressionSyntax memberAccessExpression => memberAccessExpression.Name.Identifier.Text, | ||
_ => Singleton |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since the service lifetime will have the default value of Singleton returned from the default/discard clause of the switch expression, didn't consider marking it as nullable.
Definitely, I should thank you for all your patience in suggesting the changes in making the code more readable and maintainable. And as @pictos mentioned, am looking into the possibility of making it as an incremental SG. Requires some effort, so can another review be done post that? |
I am happy to do as many reviews as needed. Is effort well spent. |
Can we add more information on the PR description about how it works, and example of generated code for example? Thanks |
Sure, will include those details. |
1 similar comment
This comment was marked as duplicate.
This comment was marked as duplicate.
namespace Microsoft.Maui.Controls.SourceGen | ||
{ | ||
[Generator(LanguageNames.CSharp)] | ||
public class RouteSourceGenerator : ISourceGenerator |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This needs to implement IIncrementalGenerator
, otherwise it will have an impact on incremental build times.
Some details here on this: dc9ac94
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This needs to implement
IIncrementalGenerator
, otherwise it will have an impact on incremental build times.
Yes, @pictos informed the same. Working on it. Will update once done and thanks for the reference link.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Have migrated to Incremental source generator.
For time being have commented out the Generator
attribute on the classic option to facilitate the review. Once the review is done, we can remove the following files related to the classic option.
- RouteSourceGenerator.cs
- RouteSyntaxReceiver.cs
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To be honest, there is a lot going on here. Can we start with a new issue that is a "proposal"?
We should iron out what the design is for this before writing code.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To be honest, there is a lot going on here. Can we start with a new issue that is a "proposal"?
@PureWeen has already opened a Discussion thread for this in the Maui CommunityToolkit repo.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will update this PR description over there too.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
return null; | ||
} | ||
|
||
private static void Execute(SourceProductionContext context, Compilation compilation, ImmutableArray<ClassDeclarationSyntax?> classes) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Your IncrementalGenerator is broken. You can't pass the Roslyn types (ClassDeclarationSyntax) to the Execute
method, they should be just in the queries pipelines.
The reason is that Roslyn types can't be compared, so there's no way to cache, and the code will be generated at every file change (no matter what file changes).
You can see the Source Generators implemented on the CommunityToolkit.MVVM or CommunityToolkit.Maui as reference
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Your IncrementalGenerator is broken. You can't pass the Roslyn types (ClassDeclarationSyntax) to the
Execute
method, they should be just in the queries pipelines.
Will check it out with the reference repo provided to restructure it.
@egvijayanand I'm going to close this for now in favor of the discussion over on MCT. CommunityToolkit/Maui#1004. Once we get through the discussion process over there then let's take next steps. I'm hoping we can get this merged into MCT so users can take advantage right away. |
Description of Change
Attribute-based shell routing and service registration in the DI container. Automated with source generators.
Below are the two attributes that are defined to generate the source that automates the tasks while defining a shell route or to add a DI service into the .NET MAUI startup pipeline.
Route attribute
This can be defined on any Page inherited type so that it can be added to the Routes collection. In addition, the type will also be added to the DI service in the startup.
By default, the route name generated would be the name of the type itself. But can be modified with its properties.
Defines a constructor to help in the generalized case with two parameters for
route
andlifetime
.The
Route
attribute defines the following properties:Route
property.ImplicitViewModel
property.MauiService attribute
This can be defined on any type so that it can be added to the Services collection. of the MauiAppBuilder instance as a DI service in the .NET MAUI startup.
Defines a constructor to help in the generalized case that takes
lifetime
as a parameter.The
MauiService
attribute defines the following properties.Output:
For each of the Shell pages defined, a partial method of the name
RegisterRoutes
with all those routes added as its method definition will get auto-generated. This method needs to be invoked in the constructor of the shell definition.Assuming MauiApp1 as the project root namespace for the sample code.
User definition:
Source generated:
For the services, a partial method of the name
ConfigureDependencies
with all those types in the Services collection as its method definition will get auto-generated. This method needs to be invoked within the CreateMauiApp() method.User definition:
Source generated:
Issues Fixed
Fixes #5312