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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Blazor] render-components-outside-of-aspnetcore - Proper DI usage #32364

Closed
wants to merge 1 commit into from

Conversation

hakenr
Copy link
Contributor

@hakenr hakenr commented Apr 20, 2024

When using dependency injection, the HtmlRenderer should not be composed directly (with new), but using a service provider.

cc @guardrex


Internal previews

馃搫 File 馃敆 Preview link
aspnetcore/blazor/components/render-components-outside-of-aspnetcore.md Render Razor components outside of ASP.NET Core

@guardrex guardrex self-assigned this Apr 21, 2024
@guardrex
Copy link
Collaborator

Thanks @hakenr ... The code was sourced from Dan's blog post here ...

https://devblogs.microsoft.com/dotnet/asp-net-core-updates-in-dotnet-8-preview-3/#render-razor-components-outside-of-asp-net-core

I think Steve built the feature. IDK if Dan wrote the code or if Steve gave it to him for publication. I'll ping them tomorrow for a look 馃憖.

@guardrex
Copy link
Collaborator

UPDATE: Dan said he'll be free to take a look at the end of the week. I haven't heard back from Steve. He might pop in for a peek at any time.

@guardrex
Copy link
Collaborator

Mmmmmm ... Nothing yet on a review. I'll leave this open a bit longer and mention it to Dan today.

@danroth27
Copy link
Member

@SteveSandersonMS Can you weigh in here on which pattern we should show? Another options seems to be removing DI completely from the sample. For example, this seems to also work: await using var htmlRenderer = new HtmlRenderer(new ServiceCollection().BuildServiceProvider(), new LoggerFactory());. Should we also consider adding a default constructor to HtmlRenderer in the framework?

@SteveSandersonMS
Copy link
Member

It's the app developer's own choice whether they want to resolve this via DI or not. Given that:

  • it only takes two constructor parameters, serviceProvider and loggerFactory
  • ... and in this sample, you want to consume it in a transient way (i.e., you manage the disposal yourself)

... there's very little benefit to obtaining the instance from DI. Arguably it has more drawbacks than benefits, since resolving anything transient from DI is a little confusing because people struggle to remember how disposal works. The standard DI interface pattern around transient+disposal is awkward so we try to avoid it.

I personally prefer the new approach in this case (making clear that the flexibility to use new directly exists here) but ultimately it doesn't matter very much.

@SteveSandersonMS
Copy link
Member

Should we also consider adding a default constructor to HtmlRenderer in the framework?

If you mean a parameterless constructor then I don't think so because of the risk of confusion. HtmlRenderer will commonly be used to render components that themselves rely on DI services, and if we default to giving them an empty service provider, they will default to throwing an exception saying that the required service could not be resolved. Empty service providers are a weird scenario so while it's fine for developers to get themselves into that situation by their own choice, I don't think we should be leading them in that direction in a subtle way.

@guardrex
Copy link
Collaborator

guardrex commented Apr 30, 2024

Thanks @SteveSandersonMS!

@hakenr ... Looks like we're leaning toward closing and going with what's in the doc. Do you have any final thoughts before we close?

@guardrex guardrex closed this May 1, 2024
@hakenr
Copy link
Contributor Author

hakenr commented May 1, 2024

@guardrex
Ah, the evil transient disposables, I see. Let's stick with the new approach then.
Should we add a note that when using dependency injection, we need to be cautious with this?

@SteveSandersonMS
Copy link
Member

Should we add a note that when using dependency injection, we need to be cautious with this?

Only if it's something unique about HtmlRenderer, but I suspect this is purely a concern about DI so we can (and already do) cover that in the DI docs.

@hakenr
Copy link
Contributor Author

hakenr commented May 2, 2024

Perhaps just a cross-reference? ;-)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants