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

[Blazor] Adds support for server side rendered forms (without a body) #47716

Merged
merged 28 commits into from
Apr 24, 2023

Conversation

javiercn
Copy link
Member

No description provided.

@javiercn javiercn requested a review from a team as a code owner April 14, 2023 20:09
@ghost ghost added the area-blazor Includes: Blazor, Razor Components label Apr 14, 2023
@@ -39,3 +50,6 @@ override Microsoft.AspNetCore.Components.EventCallback<TValue>.GetHashCode() ->
override Microsoft.AspNetCore.Components.EventCallback<TValue>.Equals(object? obj) -> bool
virtual Microsoft.AspNetCore.Components.RenderTree.Renderer.AddPendingTask(Microsoft.AspNetCore.Components.Rendering.ComponentState? componentState, System.Threading.Tasks.Task! task) -> void
virtual Microsoft.AspNetCore.Components.RenderTree.Renderer.CreateComponentState(int componentId, Microsoft.AspNetCore.Components.IComponent! component, Microsoft.AspNetCore.Components.Rendering.ComponentState? parentComponentState) -> Microsoft.AspNetCore.Components.Rendering.ComponentState!
virtual Microsoft.AspNetCore.Components.RenderTree.Renderer.DispatchEventAsync(ulong eventHandlerId, Microsoft.AspNetCore.Components.RenderTree.EventFieldInfo? fieldInfo, System.EventArgs! eventArgs, bool quiesce) -> System.Threading.Tasks.Task!
virtual Microsoft.AspNetCore.Components.RenderTree.Renderer.ShouldTrackNamedEventHandlers() -> bool
Copy link
Member Author

@javiercn javiercn Apr 14, 2023

Choose a reason for hiding this comment

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

This is to avoid allocations altogether if we are not interesting on tracking event handlers. We only track them at the time we want to dispatch an event.

We actually want to avoid tracking events altogether when we are not trying to dispatch an event. That limits/eliminates any chance we introduce breaking changes if for example, someone have today multiple forms defined on their app without giving them unique event handler names.

/// A <see cref="Task"/> which will complete once all asynchronous processing related to the event
/// has completed.
/// </returns>
public virtual Task DispatchEventAsync(ulong eventHandlerId, EventFieldInfo? fieldInfo, EventArgs eventArgs, bool quiesce)
Copy link
Member Author

Choose a reason for hiding this comment

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

We need a new overload to track quiesce when we are dispatching an event to a named event handler. Alternatively, we can make quiescence a feature the renderer offers, and in that way, we don't have to be adding it to every API.

Copy link
Member

Choose a reason for hiding this comment

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

It's surprising to me that we need to return any quiescence-related Task from here. I would have expected that when Endpoints dispatches the event, that simply extends the existing QuiescenceTask that it was going to wait for anyway.

Perhaps that doesn't work out for some reason that is hard to understand. It would be great to clean this up but I would certainly understand if you don't want to block this PR on it. So I suggest:

  • If you know clearly why it's not right for the existing QuiescenceTask to be extended by this dispatch, and don't think we should change it to do that, could you say why?
  • Or if you think it would be reasonable for the existing QuiescenceTask to be extended by this dispatch, could you file an issue saying we should do that? I'd be happy to dig into what's going on and find out if I can do that.

Copy link
Member Author

Choose a reason for hiding this comment

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

@SteveSandersonMS Everything is triggered from

protected internal async Task RenderRootComponentAsync(int componentId, ParameterView initialParameters)
{
Dispatcher.AssertAccess();
// Since this is a "render root" operation being invoked from outside the system, we start tracking
// any async tasks from this point until we reach quiescence. This allows external code such as prerendering
// to know when the renderer has some finished output. We don't track async tasks at other times
// because nobody would be waiting for quiescence at other times.
// Having a nonnull value for _pendingTasks is what signals that we should be capturing the async tasks.
_pendingTasks ??= new();

But when we wait for quiescence, we reset it after we are done (note the finally block):

private async Task WaitForQuiescence()
{
// If there's already a loop waiting for quiescence, just join it
if (_ongoingQuiescenceTask is not null)
{
await _ongoingQuiescenceTask;
return;
}
try
{
_ongoingQuiescenceTask = ProcessAsynchronousWork();
await _ongoingQuiescenceTask;
}
finally
{
Debug.Assert(_pendingTasks is null || _pendingTasks.Count == 0);
_pendingTasks = null;
_ongoingQuiescenceTask = null;
}

@javiercn javiercn force-pushed the javiercn/blazor-forms-support branch from 6fda746 to 43db13f Compare April 17, 2023 15:02
/// <summary>
/// The binding context name.
/// </summary>
[Parameter] public string BindingContextId { get; set; } = default!;
Copy link
Member

Choose a reason for hiding this comment

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

What's the difference between Name and BindingContextId? Developers might struggle to understand this as they sound like the same thing (also: the XML docs are duplicated).

Is there possibly any other name that could differentiate these more clearly?

@javiercn javiercn force-pushed the javiercn/blazor-forms-support branch from 1bdbc70 to 27508dc Compare April 19, 2023 15:48
@javiercn
Copy link
Member Author

I've done a last round of cleanups and pushed and update. I might add a couple more streaming rendering tests, but otherwise, it should be good to go % feedback.

Copy link
Member

@MackinnonBuck MackinnonBuck left a comment

Choose a reason for hiding this comment

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

LGTM, just some small questions/suggestions.

@javiercn javiercn force-pushed the javiercn/blazor-forms-support branch from 22bd253 to ed05d7e Compare April 21, 2023 08:17
@javiercn javiercn merged commit 442d656 into main Apr 24, 2023
@javiercn javiercn deleted the javiercn/blazor-forms-support branch April 24, 2023 15:55
@ghost ghost added this to the 8.0-preview4 milestone Apr 24, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-blazor Includes: Blazor, Razor Components
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants