Skip to content

[Components] Adds support for rendering async components #6708

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Jan 17, 2019
Merged

[Components] Adds support for rendering async components #6708

merged 1 commit into from
Jan 17, 2019

Conversation

javiercn
Copy link
Member

  • Converts SetParameters to SetParametersAsync
  • Captures all the returned task produced by SetParametersAsync of each component to properly await them.
  • Introduces RenderRootComponentAsync to render the root component and deterministically wait for all async work to be done.
  • Protects the state in Renderer from concurrent modifications.

@vertonghenb
Copy link
Contributor

Could this fix the following issue? #5592 (comment)

@rynowak
Copy link
Member

rynowak commented Jan 15, 2019

Question I forgot to ask elsewhere, how will this handle encoding?

@javiercn
Copy link
Member Author

Question I forgot to ask elsewhere, how will this handle encoding?

@rynowak The HTML renderer gets a Func<string,string> encoder that does the encoding. Due to the fact that this lives in Microsoft.AspNetCore.Components and we don't want to bring a dependency to the webencoders in that package.

Mvc just provides the htmlEncoder resolved from DI

@rynowak
Copy link
Member

rynowak commented Jan 15, 2019

Ok cool beans

@javiercn javiercn force-pushed the javiercn/components-async branch from d2577cd to a67f314 Compare January 15, 2019 23:20
SteveSandersonMS
SteveSandersonMS reviewed Jan 16, 2019
View reviewed changes
src/Components/src/Microsoft.AspNetCore.Components/Rendering/Renderer.cs Outdated
@@ -22,6 +23,14 @@ public abstract class Renderer : IDisposable
private int _nextComponentId = 0; // TODO: change to 'long' when Mono .NET->JS interop supports it
private bool _isBatchInProgress;
private int _lastEventHandlerId = 0;
private List<Task> _pendingTasks = new List<Task>();
Copy link
Member

Choose a reason for hiding this comment

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

I might be missing something, but it looks like in the non-HtmlRenderer case, this list will grow infinitely and is never cleared after the initial render.

Should we do something so that only HtmlRenderer actually populates this list, and the others do nothing in AddToPendingTasks? The AddToPendingTasks could be changed to a virtual method that no-ops in the default case.

Copy link
Member Author

Choose a reason for hiding this comment

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

This gets cleaned up in all code paths as

void RenderRootComponent(int componentId, ParameterCollection initialParameters)
{
            ReportAsyncExceptions(RenderRootComponentAsync(componentId, initialParameters));
}

Internally makes sure that all the cleanup happens.

Copy link
Member

Choose a reason for hiding this comment

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

I thought that only happened on the first render cycle. When some event like a button click happens later in the other renderers, this code path doesn't execute, does it?

Copy link
Member Author

Choose a reason for hiding this comment

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

Ah, I see what you mean. You're right. But it has an easy fix

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 null the list after RenderRootComponentAsync is done

SteveSandersonMS
SteveSandersonMS reviewed Jan 16, 2019
View reviewed changes
Copy link
Member

@SteveSandersonMS SteveSandersonMS left a comment

Choose a reason for hiding this comment

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

Looking great! Hopefully the remaining points won't be hugely disruptive to resolve. Thanks for tracking down so many fine details about the lifecycle method execution process.

@javiercn
Copy link
Member Author

Updated

SteveSandersonMS
SteveSandersonMS reviewed Jan 16, 2019
View reviewed changes
src/Components/src/Microsoft.AspNetCore.Components/ComponentBase.cs Outdated
// reason we have OnAfterRenderAsync is so that the developer doesn't
// have to use "async void" and do their own exception handling in
// the case where they want to start an async task.
var taskWithHandledException = HandleAfterRenderException(onAfterRenderTask);
Copy link
Member

Choose a reason for hiding this comment

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

Looks like we don't need the taskWithHandledException var, or do we?

Copy link
Member Author

Choose a reason for hiding this comment

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

The compiler complains if we take it out

Copy link
Member

Choose a reason for hiding this comment

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

_ = HandleAfterRenderException(...);

SteveSandersonMS
SteveSandersonMS approved these changes Jan 16, 2019
View reviewed changes
Copy link
Member

@SteveSandersonMS SteveSandersonMS left a comment

Choose a reason for hiding this comment

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

Excellent!

@javiercn
[Components] Support for prerrendering asynchronous components.
e7892e1 
* Updates the IComponent interface to rename Init into Configure
* Updates the IComponent interface to change SetParameters for
  SetParametersAsync and make it return a Task that represents when the
  component is done applying the parameters and potentially triggering
  one or more renders.
* Updates ComponentBase SetParametersAsync to ensure that OnInit(Async)
  runs before OnParametersSet(Async).
* Introduces ParameterCollection.FromDictionary to generate a parameter
  collection from a dictionary of key value pairs.
* Introduces RenderComponentAsync on HtmlRenderer to support
  prerrendering of async components.
* Introduces RenderRootComponentAsync on the renderer to allow for
  asynchronous prerrendering of the root component.
@javiercn javiercn force-pushed the javiercn/components-async branch from e2c8696 to e7892e1 Compare January 17, 2019 07:42
@javiercn
Copy link
Member Author

This is blocked waiting on this unrelated failing test:
2019-01-17T09:17:29.8475061Z [xUnit.net 00:00:50.51] Microsoft.DotNet.Watcher.Tools.Tests.MsBuildFileSetFactoryTest.FindsCustomWatchItems [FAIL]
2019-01-17T09:17:32.4442223Z Failed Microsoft.DotNet.Watcher.Tools.Tests.MsBuildFileSetFactoryTest.FindsCustomWatchItems

@natemcmaster Is there something we can do here? I've triggered several builds already

@javiercn javiercn force-pushed the javiercn/components-async branch from d2bf15b to e7892e1 Compare January 17, 2019 13:53
@rynowak
Copy link
Member

rynowak commented Jan 17, 2019

If the flaky test is blocking you, log an issue and mark it as skipped.

@mkArtakMSFT mkArtakMSFT merged commit 19b543e into master Jan 17, 2019
@mkArtakMSFT
Copy link
Contributor

mkArtakMSFT commented Jan 17, 2019 •
edited
Loading

Merging in as this is blocked on an unrelated test.

@natemcmaster natemcmaster deleted the javiercn/components-async branch January 18, 2019 19:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rynowak rynowak rynowak left review comments

@campersau campersau campersau left review comments

@Rad2417 Rad2417 Rad2417 left review comments

@SteveSandersonMS SteveSandersonMS SteveSandersonMS approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@javiercn @vertonghenb @rynowak @mkArtakMSFT @SteveSandersonMS @campersau @Rad2417