-
Notifications
You must be signed in to change notification settings - Fork 10.3k
Blazor API Review: Compiler Services #11907
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
Comments
Fixes: #11907 These are the changes from the compiler-related API review that are safe to make (these methods are not used). I'll follow up with `BindMethods.GetValue` separately because that's used by the compiler.
I notice there are some thoughts on removing code that is not used since the introduction of EventCallback. Can I just check that removing those methods will not prevent the use of Action for ValueChanged in component binding? |
It will not prevent this. If you have some sample code that you're interested in, I'm happy to try it out with this change while it's still in PR. |
Thanks I'm just thinking of this sort of thing https://github.com/SQL-MisterMagoo/BlazorEventsDemo/blob/master/BlazorEvents/Comps/ActionValue.cs That file is in is a preview6 Blazor project to compare the use of Action vs EventCallback , so could be used as a sample if you wanted. |
Is that true even for the |
Excellent! |
Everything else here sounds great. |
The |
Are they good candidates to move to |
Yes, I think these could also be moved. |
1st part of this has been merged, reopening because there's still a little more cleanup to do here. |
This will be done once #12250 is merged |
Closing this. The changes needed have been made. |
Summary
These are APIs that are used mostly by the compiler - some are used exclusively by the compiler. I'm err-ing on the side of safety here by including things that the compiler touches so that we look at them.
At a high-level I plan on parking compiler-specific things in
Microsoft.AspNetCore.Components.CompilerServices
- similar to what the BCL does.The primary thing to figure out with all of these APIs is whether there's any use case for a user to call them, or if they are purely an artifact of the compiler.
APIs
All of the
GetEventHandlerValue
methods can be removed, they are unused since we introducedEventCallback
.GetEventHandlerValue
methodsThe
GetValue
methods will be used bybind
when we need to convert a value to a string. These needs to have some further design iteration. For now we need to answer whether these will be used by user-code or not. I think we need to err on the side of making them usable.BindMethods.GetValue
The
EventCallbackFactory
methods are intended to be used from C# components as well as by the compiler. We generally need to use the C# compiler to resolve overloads when we have polymorphism. These need to be extension methods so that the compiler can use them, and so we can split them across assemblies. This allows us to separate the methods used for the DOM from Blazor native.We'll move the
EventCallbackFactoryUIEventArgsExtensions
class toM.A.Components.Web
as part of that review.I'm not proposing any changes to any of this as part of this work item. Other opinions?
This is purely a compiler implementation detail.
M.A.C.CompilerServices
I'm going to keep the name. You read that right. Ryan is naming something "Helpers". This is because the term is appropriate - this is a random pile of static "Helper" methods that the compiler needs - and that's the right design. And it's symmetric with this
I'm still going to argue against creating "Helper" class in all other situations 😆
These should be deleted. These were used before we introduced
EventCallback
and aren't used anymore.UIEventArgsRenderTreeBuilderExtensions
The text was updated successfully, but these errors were encountered: