Skip to content
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

Support the new top level statements with WebApplicationFactory #33462

Merged
merged 6 commits into from
Jun 11, 2021

Conversation

davidfowl
Copy link
Member

@davidfowl davidfowl commented Jun 11, 2021

  • This change adds support for the WebApplicationFactory without a CreateHostBuilder method. It uses the new ResolveHostFactory method in combination with a DeferredHostBuilder and DeferredHost to accomplish this.
  • This works pretty differently from the existing pattern for a couple of reasons:
  • We don't know when the application is done with configuring because that code is running in main. Main can basically do anything so we need to wait for signals in order to make progress (events that fire or calls that get made along the execution path). As a result, when the IHostBuilder is built, it manipulates the application's IHostBuilder and when the application calls Start on the IHost, the WebApplicationFactory waits for this to happen. Instead of the WebApplicationFactory creating a clone of the application's "context", it's mutating it on the fly.

TODO:

  • I need this change to follow up on a few edge cases with this approach Follow up on HostFactoryResolver changes runtime#54052 around the application exiting without calling Start (it'll currently hang). This will be a follow up PR since we need to wait on dependency flow.
  • Now: Tests, I'll add a test for the new approach.

PS: One downside with top level statements is that there's no publicly accessible type to reference the entry point, so application developers need to create a dummy type to reference it at compile time. cc @jaredpar @MadsTorgersen

- This change adds support for the WebApplicationFactory without a CreateHostBuilder method. It uses the new ResolveHostFactory method in combination with a DeferredHostBuilder and DeferredHost to accomplish this.
@ghost ghost added the area-mvc Includes: MVC, Actions and Controllers, Localization, CORS, most templates label Jun 11, 2021
- Look for both IHostBuilder and IWebHostBuilder before falling back to the new pattern.
{
return builder.UseEnvironment(Environments.Development);
return new NullWebHostBuilder();
Copy link
Member Author

Choose a reason for hiding this comment

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

I could also change the nullability here but I couldn't figure out how to do that. All of the tooling was fighting me.

Copy link
Member

Choose a reason for hiding this comment

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

So changing the method signature to:

protected virtual IWebHostBuilder? CreateWebHostBuilder()

Is not working here? Bummer.

I say that mostly because it tripped me up while reading it. Why are we creating a NullWebHostBuilder? Well, that doesn't actually matter, we're just saying there's no builder on the target assembly so we need to construct our own deferred one. It would've read better with nullability set correctly here. If it's possible to do that, I think it'll help us with maintainability for this moving forward.

Copy link
Member Author

Choose a reason for hiding this comment

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

I would actually rather change it to be that but I couldn't get the public API files working. I would need somebody to explain the error I was getting 😄 . Also I don't know what the bar is for breaking nullability changes.

cc @pranavkm @JamesNK

Copy link
Contributor

Choose a reason for hiding this comment

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

All of the tooling was fighting me.

I'm guessing it's the publicapi analyzer?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, I was raging at this last night and I went to bed after outsmarting nullable 😄

Copy link
Member

@javiercn javiercn left a comment

Choose a reason for hiding this comment

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

Looks great!

Get the tests to pass!

@davidfowl davidfowl marked this pull request as ready for review June 11, 2021 15:47
{
// Copy the properties from this builder into the builder
// that we're going to receive
foreach (var pair in Properties)
Copy link
Member

Choose a reason for hiding this comment

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

Why not do:

b.Properties = new Dictionary<object, object>(Properties)

here?

Copy link
Member Author

Choose a reason for hiding this comment

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

b.Properties is read only.

{
return builder.UseEnvironment(Environments.Development);
return new NullWebHostBuilder();
Copy link
Contributor

Choose a reason for hiding this comment

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

All of the tooling was fighting me.

I'm guessing it's the publicapi analyzer?

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