Skip to content

tweak that simplifies Worker startup code - #43056

Closed
bradygaster wants to merge 1 commit into
mainfrom
bradyg/worker-tweak-for-7
Closed

bradygaster wants to merge 1 commit into
mainfrom
bradyg/worker-tweak-for-7

Conversation

@bradygaster

Copy link
Copy Markdown
Member

Do not merge yet. Reviewing with @davidfowl

@bradygaster
bradygaster requested a review from Pilchie as a code owner August 2, 2022 21:22
@ghost ghost added the area-mvc Includes: MVC, Actions and Controllers, Localization, CORS, most templates label Aug 2, 2022
@davidfowl

Copy link
Copy Markdown
Member

One thing to discuss @halter73 and @eerhardt is that we didn't expose IHostBuilder on the host application builder. I think that will break things like serilog and others that don't have IServiceCollection or IApplicationBuilder extensions methods. It probably also break orleans right @ReubenBond ?

@halter73

halter73 commented Aug 2, 2022 •

Copy link
Copy Markdown
Member

A HostApplicationBuilder adapter does exist. It is exposed through a diagnostic listener for testing and tooling scenarios, but we could not get it approved as a public API.

The problem with exposing an IHostBuilder adapter is that the HostApplicationBuilder cannot do everything an IHostBuilder can after it's constructed. Some things like the ContentRootPath that cannot be changed, so it wouldn't be compatible with every IHostBuilder extension method, but it might be compatible with most. I'm not sure. It's annoying that a lack of support turns into a NotSupportedException at startup, but since this would be new API, maybe that's okay.

During the HostApplicationBuilder API review, we suggested that extension methods that currently target IHostBuilder instead target IConfigurationBuilder and/or IServiceCollection directly. This might mean sometimes splitting up the method. You could write an extension method targeting HostApplicationBuilder, but we don't recommend that because it's sealed.

@@ -1,10 +1,7 @@
using Company.Application1;

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Isn't Worker still in this namespace?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes duh

@ReubenBond

Copy link
Copy Markdown
Member

It probably also break orleans right @ReubenBond ?

It breaks 3.x, yeah. In 4.x, we have a method to add/configure Orleans via IServiceCollection instead of only IHostBuilder

@davidfowl

Copy link
Copy Markdown
Member

We may need to hold on this one, a quick browse shows that some important 3rd party packages have built extensions on IHostBuilder.

@davidfowl

Copy link
Copy Markdown
Member

Yep, I'm convinced this is a bad decision for 7.0, nothing will work with this new builder so we should give people time to adopt it before making everyone revert to the old builder because it doesn't work with their package.

@bradygaster bradygaster closed this Aug 2, 2022
@halter73

halter73 commented Aug 2, 2022

Copy link
Copy Markdown
Member

I doubt migrating from the generic host to HostApplicationBuilder will have the same wow factor as migrating from the generic web host to WebApplicationBuilder. Not having to use callbacks is nice, but I don't expect many people to switch just for that.

Without using it in the templates and docs, I don't see many people migrating. I think it's okay if most people continue to use HostBuilder though. HostApplicationBuilder has allowed us to clean up WebApplicationBuilder and fix a lot of bugs, and the API is still public for those that prefer less callbacks.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area-mvc Includes: MVC, Actions and Controllers, Localization, CORS, most templates

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants