-
Notifications
You must be signed in to change notification settings - Fork 10.3k
Support IServiceProviderFactory in WebAssemblyHostBuilder (imported from Blazor PR 1623) #4785
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
Conversation
@rynowak Another one I've already reviewed and consider ready, but need another approval before it can be merged. Please feel free to review further if you want! |
} | ||
|
||
return provider; | ||
} |
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.
@davidfowl @Tratcher is this right? I thought we exposed the service provider factory through an extra method on the hosting builder.
Think of this code as aligning with the requirements of generic host in 3.0 rather than web host.
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.
You don't want to build the container twice. See:
https://github.com/aspnet/Extensions/blob/ea0df662ab06848560d85545f3443964c57e9318/src/Hosting/Hosting/src/HostBuilder.cs#L198-L205
And
https://github.com/aspnet/Extensions/blob/ea0df662ab06848560d85545f3443964c57e9318/src/Hosting/Hosting/src/HostBuilder.cs#L79-L89
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.
OK, thanks for the info!
I've updated it to follow the same patterns as in those examples. It seems to fit in fine, since the web assembly host builder has direct equivalents to each concept. Does the updated code look right to you, @Tratcher and @rynowak?
If there are any deeper complications, we don't strictly have to do any of this PR at all. It's not been a widely-requested feature, and could be re-attempted at some later date if we want.
c0b11ba
to
50a2d71
Compare
Looks fine. At a higher level I wonder why you're selectively copying IWebHostBuilder/IHostBuilder rather than moving entirely to IHostBuilder? |
We can't afford the extra 350kb of dependencies that generic host brings in for client-side Blazor. I'll see if I can dig up the thread where we evaluated this. Most of the dependencies can't be linked out because of the way this is all put together. |
Is that with Ms.Ex.Hosting or Ms.Ex.Hosting.Abstractions? I wonder if IHostBuilder would be suitable for you even if HostBuilder is not? |
That would be ideal. That way extension methods would still work. We should also look at the sub builder pattern to see if that makes sense here |
Re: I'll merge this PR as-is, since it follows on from the established patterns. |
385kb with just abstractions. It includes all of the abstractions for the other |
We cam proably from the System.Memory one. That’s primitives pulling it in because of change tokens. |
Imported from dotnet/blazor#1623