conficient / BlazorTemplater

A library that generates HTML (e.g. for emails) from Razor Components
Apache License 2.0
146 stars 16 forks source link

Add AsyncComponentRenderer - fixes #23 #30

Closed Drake53 closed 1 year ago

Drake53 commented 2 years ago

Basically a copy of Htmlizer.cs, using Renderer.RenderRootComponentAsync() to properly await any async work being done in components.

conficient commented 2 years ago

Thanks for the PR.. just looking at this to see what it does and how it works. There are no tests and no examples of usage, so I can't accept the PR into the main app for those reasons. However, it does give me a point to try to integrate into the existing code.

Ideally what we need is a method on ComponentRenderer such as .RenderAsync() - so I'll see if I can adapt the code to make that work.

Drake53 commented 2 years ago

I understand you'd prefer to add the method to the existing class, but from what I understand its code is based on bUnit so it has a bunch of stuff you don't actually need if you only want to get the HTML. I didn't see a way to add RenderAsync to it while keeping the implementation as simple as it is now so I made a separate class. You could probably keep using Htmlizer instead of copying it into the class, if you made some changes (like changing HtmlRenderingContext.Renderer type), though IMO it would be better to keep it separate and maybe mark the old class obsolete, because you should always prefer to render async.

Example usage:

var parameters = new Dictionary<string, object>()
{
    { nameof(MyComponent.Model), new Model() { Value = "Test" } },
    { nameof(MyComponent.Title), "Test" },
};

var renderer = new AsyncComponentRenderer<MyComponent>(serviceProvider, loggerFactory);
var html = await renderer.RenderAsync(parameters);

Note that I'm not sure you can use nameof() directly on the component, in my own code I have the component implement an interface.

I can add tests as well, just wanna make sure you approve of the usage/API first.

Drake53 commented 1 year ago

I have pushed some changes since there were some issues with the initial implementation. I was initially rendering the component type directly, but doing this skips DI causing all injected properties to be null. To fix this I am now rendering a LayoutView, with the added benefit that layouts are now supported as well. I have also added tests, these were copied from ComponentRenderer_Tests, and I added one additional test that shows it's possible to render multiple times.

PhotoAtomic commented 1 year ago

Hello! I was working on the same thing, and then I've noticed that there was already a PR for the same topic. Anyway the solution proposed here looks a little complicated.

I will look with more attention but from a first look it seems that the synchronization is made with a newly introduced semaphore. Why not just use the "NextRender" task already existing in the HtmlRenderer? It looks easier and cleaner and doesn't introduces new concepts. I have a PR that uses it to implement the RenderAsync method and looks working great. Maybe I'm missing something?

Drake53 commented 1 year ago

I tried a couple of use cases, and noticed that RenderRootComponentAsync requires an async lock (SemaphoreSlim). If you always create a new renderer for one render, then you don't need it of course, but I wanted to support every use case I could think of.

This library was based on a unit testing library, and in ContainerComponent.cs I read:

// This also avoids the use of Renderer's RenderRootComponentAsync APIs, which are
// not a good entrypoint for unit tests, because their asynchrony is all about waiting
// for quiescence. We don't want that in tests because we want to assert about all
// possible states, including loading states.

A lot of the code in this library is to support the unit testing use case. A use case that doesn't apply to this library. And this comment tells us that we can instead use the built-in RenderRootComponentAsync. So that's what I did.

You mentioned that "NextRender" already exists. This also seems like an artifact from being based on a unit testing library, and I feel like your solution won't work if you put multiple delays in OnInitializedAsync (you may need to manually call StateHasChanged, but I suspect if you tried to make a test for this it wouldn't work).

So my solution may look complicated but I think that's just because I use a different API so there's more change, while your solution integrates better with the existing code.

PhotoAtomic commented 1 year ago

Yep, NextRender appear to be marked as completed (and then substitued with a new task) each time an update on the page is triggered. So for the first "run" it waits for all the async lifecycle task to complete.

I also agree that this code derives from a testing library and I've also tryed to use the RenderRootComponentAsync method too (i've noticed the same comment), but as you said it required a lot of changes in the code.

That other solution (PR #33 ) have a very low impact and integrates well with already tested features. A possible solution could be to get rid of all the code that was written for the testing purpose and redesign the library wit the intent to be just a template generator. It could be interesting and could open up to new feature as well.

About my solution, given the place where i've awaited NextRender it is waits until the OnInitializeAsync is completed, no mater how many delay I await inside it, looks like that also the other async lifecycle methods are respected as well but i've not tested them explicitly. I'll do other test in the next days to see if there are any edge cases. EDIT: as you noticed here my solution doesn't work if StateHasChanged is called during the Async method because it generates one more NextRender task, completing the one i'm waiting on too early. I've rushed my solution too much, thanks for having spotted it. So, if yours covers also this scenario, I think it is the way to go. Can we progress in having your merged? what is missing?

Anyway, which are the other scenario your solution can cover? i can imagine a template being useful with async data retrievel only when tey occur in one of the "first" async lifecycle method (the first render, the async parameter set, and the async init)

other than that, how can you know for sure when the page have finished grabbing things?

maybe i'm thinking just to email and report generation, and you need to use it in scenario when there is user interaction and take snapshots?

Drake53 commented 1 year ago

Seems like this library will become obsolete in .NET 8 https://devblogs.microsoft.com/dotnet/asp-net-core-updates-in-dotnet-8-preview-3/

conficient commented 1 year ago

Yes, I think that will be the case, although I'll leave it alone for anyone using .NET Core 3.x - 7 as they will be able to use it.