Remove dictionary conversion from SetParametersAsync source generator (#9682) #10205
Conversation
…ensures that when passing ParameterView to base classes like InputBase<TValue> cascaded parameters are not lost.
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the WalkthroughThis update simplifies how parameters are handled in the Changes
Sequence Diagram(s)sequenceDiagram
participant Client as Caller
participant Generator as BlazorSetParametersSourceGenerator
participant Base as BaseComponent
Client->>Generator: Call SetParametersAsync(parameters)
Generator->>Generator: Process parameters (no dictionary removal)
Generator->>Base: Call base.SetParametersAsync(parameters)
Base-->>Generator: Return result
Generator-->>Client: Return result
Poem
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/SourceGenerators/Bit.SourceGenerators/Blazor/BlazorSetParametersSourceGenerator.cs (1)
86-86: Improvement to parameter inheritance handling.This change correctly implements the PR objective by passing the original
parametersto the base class'sSetParametersAsyncmethod rather than creating a newParameterViewfrom the filtered dictionary. This ensures parameters are properly passed up the inheritance hierarchy and avoids reflection-based initializations that could degrade performance.The previous approach unnecessarily removed parameters after processing them and then created a new
ParameterView, which would have prevented parent components from accessing these parameters.Consider adding a comment explaining the rationale behind this approach to help future maintainers understand the design decision:
else { + // Pass the original parameters up the inheritance hierarchy to ensure parent components + // can access all parameters and avoid reflection-based initializations source.AppendLine(" return base.SetParametersAsync(parameters);"); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/SourceGenerators/Bit.SourceGenerators/Blazor/BlazorSetParametersSourceGenerator.cs(1 hunks)
|
Greetings! parameters.TryGetValue<string>("Prop1", out Prop1);
parameters.TryGetValue<int>("Prop2", out Prop2); |
|
@ShahryarSaljoughi thanks for submitting this PR. to have the best performance in this method, it's better to follow the instruction provided by the Microsoft: so plz remove the conversion to Dictionary and use the ParameterView directly. Note: using the TryGetValue is not appropriate here, since it will iterate (foreach loop) over elements for each call. |
|
@msynk I'm on this, I'll come up to you in 2 hours. Thank you for your invaluable feedback, this is so good. |
|
@msynk Hi. I added a new commit. The new commit simply avoids conversion to dictionary and directly uses the |
msynk
changed the title
SetParametersAsync source generator so that ParameterView climbs up inheritance hierarchy AS IS
closes #9682
Behaviour after this change:
This PR implements the changes we previously discussed with maintainers. Here is a brief of the implemented solution:
SetParametersAsyncmethod, initializes parameters defined within the enclosing class (does not initialize properties inherited from base type, or properties of the runtime type, that are not available in current type) and then calls the base implementation ofSetParametersAsyncmethod.Microsoft.AspNetCore.Components.ComponentBase, the generatedSetParametersAsyncpasses an emptyParameterViewto base implementation, This is correct because parameters defined in derived types are already initialized during the call chains and theComponentBaseitself has nothing to initialize using theParameterViewMicrosoft.AspNetCore.Components.ComponentBase, the generatedSetParametersAsynpasses the sameParameterViewit receives to its parent class if the parent class is notComponentBase. (Does not delete values already assigned to parameters from theParameterViewanymore)We prevented slow code, with one exception:
Our intention was to prevent the execution of reflectoin-based initializations which may happen if
Microsoft.AspNetCore.Components.ComponentBasereceives a not emptyParameterViewin itsSetParametersAsync.There is only one scenario in which that slow code gets executed. If our component is neither directly inheriting from
Microsoft.AspNetCore.Components.ComponentBasenor is covered by our source generator (maybe because it is not in our assembly), then that slow reflection based initialization is going to execute. The following shows an example of such scenario:classDiagram ComponentBase <|-- ExternalComponent ExternalComponent <|-- MyComponentMyComponent is covered by source generator.
ExternalComponent is in a third-party assembly and not covered by source generator.
ComponentBase is
Microsoft.AspNetCore.Components.ComponentBase.Summary by CodeRabbit