wip/experiment: new renderer

[egil]

Pull request description

Work in progress!

This should implement a new way of rendering components that allows us to create DOM tree directly using AngleSharps API, as well as create our own component tree that we can navigate. This PR contains a very naïve port of BrowserRenderer.ts and related types from Blazor. There are definitely optimizations and improvements left on the table.

Open questions:

Current it contains a bunch of experiments I am looking for feedback on:

• Side effect on DOM on event handler trigger

It is possible to update the DOM when an event is triggered in the attached event handler. However, it would be best if we can figure out a way to get AngleSharp to do it for us, so bUnit doesn't have to reimplement the behavior of the browser.
If it has to be bUnit thing, it should only happen once, even if there are multiple event handlers attached to the same element, e.g. two onkeypress on an input element or one onkeyup and one onkeydown.

• Detached elements/nodes user handling

When a node/element is removed from the DOM during a re-render, should the node go into a "detached" state, which prevents users from using it? E.g. if a user has a local reference to the node in their test, and then after it has been removed, try to access one of its properties, should we throw an exception?

• Should nodes be read only for users?

Should we prevent users from modifying the DOM tree? If they do, things might blow up on them, like it would if they modify it in the browser with JavaScript.

Related issue: #46

Closed questions:

• Should we include a TestRenderer.RenderAsync, which blocks until all async life cycle methods complete?

It is possible to create a RenderAsync method in TestRenderer that blocks until there are no async incomplete tasks in the render queue. Should such a method be included or will it just confuse users. If a user uses this method, they will not be able to test rendered markup/state of a component before an async operation completes, e.g. the "loading data scenario".

In some cases, it might just be simpler for users to call var cut = await RenderAsync(renderFragment) if their test is not focused on the particular async stuff in their life cycle methods, but more on the outcome, e.g. the DOM tree after the full render is complete. If they use RenderAsync, they do not have to do a WaitForXXX call to ensure their DOM is in a predictable state.

There would still need to be a Render(renderFragment) that returns the IRenderedComponent directly, such that users are able to test "partial first renders", e.g. the "loading scenario".

• Support Blazor's event types and async event handlers through AngleSharp event system

AngleSharp's Event, EventTarget.AddEventListener, and EventTarget.Dispatch is not compatible with Blazors custom EventArgs types, but they do correctly implement the bubbling etc., so it makes sense to reuse that, instead of reimplementing bubbling etc., like in v1.

To allow users to await async event handlers the proposal is to have a custom AngleSharp Event type named BunitEvent, that carries the Blazor event args type with it, and can collect a list of Task from async event handlers that it can expose as a Task EventDispatchResult => Task.WhenAll(eventDispatchResults) property.

Then the current event trigger extensions methods from v1, e.g. Click(), can be recreated to dispatch BunitEvents with the correct Blazor event args inside, and can have ClickAsync() counter parts, that are await the EventDispatchResult property.

---

Closes #48.

[linkdotnet]

Should we include a TestRenderer.RenderAsync, which blocks until all async life cycle methods complete
It is possible to create a RenderAsync method in TestRenderer that blocks until there are no async incomplete tasks in the render queue. Should such a method be included or will it just confuse users. If a user uses this method, they will not be able to test rendered markup/state of a component before an async operation completes, e.g. the "loading data scenario".

In short: Yes I am in favor of that. For two reasons:

1. Convenience: Often times when you create a component which has async logic in OnInitializedAsync you just care for the outcome not the steps in between. Making the test code more compact. I guess this is the average case. You will have maybe 1 test checking what OnInitializedAsync does (like smaller substeps which updates a loading bar) but every other case doesn't need that (removing the need for WaitForXXX before invoking let's say a button click).
2. I do tend to say it's normal to have async tests in the ASP.NET Core world.

[egil]

In short: Yes I am in favor of that. For two reasons: ...

I am also leaning towards yes. This will also seem consistent with async versions of the WaitFor methods, and this would actually not be a breaking change from v1, since Render would still be there.

[egil]

Pause reviewing a bit for now.

I am current just throwing stuff against the wall to understand the inner workings of the renderer. What I am working on now is a port of the TypeScript based renderer. Its not too far from what I have been doing so far, but there are parts I would like to avoid in it, but basing my efforts on the Mobile Blazor Bindings didn't work out, so I am trying that now. The problem with MBBs approach is that they assume that there can only be a single node or element per render frame, but that's not something we control, since we're supporting components written in for HTML, where that is not the case.

[egil]

Status update: started porting the browser rendering logic in https://github.dev/dotnet/aspnetcore/blob/main/src/Components/Web.JS/src/Rendering/. Right now I try my best to do a 1-to-1 port, even if the code is not at all idiomatic C# and efficient. Then once, everything is work, ill take a stab at tweaking things, while keeping the code close to the original to make it easy to keep in sync.

@linkdotnet if you are familiar with TypeScript, this could use some a sanity check (if you have the time - lots of code to look through - don't sweat it if you don't have time). The relevant files are all under bunit.web/RenderingPort/. I have simply been sitting with the TS code open in one window next to a window with the C# port and gone through it line by line. I still have parts missing from AngleSharpRenderer and EventTypes, but still managed quite a lot so far.

Ps. all tests in \tests\bunit.web.tests\RenderingPort\AngleSharpRendererTest.cs runs now. Its not a lot, but CanTriggerEvents exercises quite a bit of the code.

[linkdotnet]

That sounds more like a boolean flag. How about: DispatchTask or CompletedDispatchTask?

[linkdotnet]

One a second note: Does it need that Task object in the first place?
So we will have a Task<IEventDispatchResult> which has a Task DispatchComplete.
So the outer task can be successful but the inner one can be in a faulted state?

[egil]

One a second note: Does it need that Task object in the first place?

I think it makes sense that both async and sync trigger methods return the same type.

So we will have a Task<IEventDispatchResult> which has a Task DispatchComplete.
So the outer task can be successful but the inner one can be in a faulted state?

DispatchCompleted no I think they can only be in the same state.

[linkdotnet]

I think it makes sense that both async and sync trigger methods return the same type.

Oh don't get me wrong, that makes absolute sense. I am just curious if we need the property Task DispatchComplete especially if they are in the same state all the time (from an outside / consumer point of view).

The only use case might be to check for a faulted state in a sync version, but even then we should receive an exception first.

[egil]

Another use case could be to await it later, e.g. call Click, then await the property at a later stage in the test. There is a test included here that actually does that. That could also be done with ClickAsync obviously.

Perhaps we dont have a ClickAsync test, and instead let users await DispatchCompleted instead, e.g. await Click().DispatchCompleted.

But I see your point, its mixing concepts a bit. Perhaps the interface should simply contain a boolean that reads the task status and relay that.

[linkdotnet]

Another use case could be to await it later, e.g. call Click, then await the property at a later stage in the test. There is a test included here that actually does that. That could also be done with ClickAsync obviously.

Perhaps we dont have a ClickAsync test, and instead let users await DispatchCompleted instead, e.g. await Click().DispatchCompleted.

But I see your point, its mixing concepts a bit. Perhaps the interface should simply contain a boolean that reads the task status and relay that.

I see. I see. For that obviously I would take the ClickAsync method instead of the sync version and then await it later. Especially because it will be done via Wait or Result which can be dangerous in tests. I would prevent users from doing that in the first place.

If someone is experienced he can still use ClickAsync and await it synchronously .

[egil]

@linkdotnet this is an example where we need to await tickClick completion later. Here I use var tickClick = cut.Find("#tick").ClickAsync(); and then further down await tickClick;.

I could also have done var tickClick = cut.Find("#tick").Click() and then later await tickClick.DispatchCompleted.

The reason why I liked the name DispatchCompleted for that Task property is that it sort of reads nicely, I think. "await tick click dispatch completed".

As per your previous comment, I don't want to encourage users to use tickClick.DispatchCompleted.Wait() or similar bad practices. Dont expect folks will either. Most know not to, and when they see a Task, they know what to do.

Does this change your mind wrt. the name of the property on IDispatchResult?

[linkdotnet]

I am totally fine with that.
I do feel the first approach just feels more natural and what one is used from Task objects.

Personally I would almost always prefer the first approach over the second. But that is just me, so please keep it.

[egil]

Me too. First approach feels more native to C#.

I will sleep a bit on whether or not to surface the Task DispatchCompleted property in IDispatchResult, or if IDispatchResult should just have a bool IsDispatchCompleted property instead, that will indicate to a user who is using Click() whether or not dispatch has completed at the point where the code returns to the test. Then they would know whether or not they might need to use ClickAsync() to get a task and await that or not.

One argument for keeping bool IsDispatchCompleted in IDispatchResult is that we might add information in that is readily availably like IsDefaultPrevented or a list of nodes who's event triggers was invoked. That information might be useful to the user prior to the user wanting to await the completion of the dispatches async event handlers.

[egil]

Hi @SteveSandersonMS, a quick question related to this.

To save you from reading everything here, this is the TLDR: We're experimenting with porting BrowserRenderer.ts and related types/classes from Blazor, since it looks like building and maintaining the DOM tree rather than regenerating it completely at every render will enable some interesting features for us.

Since the TypeScript code/BrowserRenderer is pretty complex, I want to make sure we get everything ported correctly. I have finished porting all Selenium tests in dotnet/aspnetcore/src/Components/test/E2ETest/Tests/ComponentRenderingTestBase.cs, and would appreciate if you could point out other, if any, potential test (suites) that are relevant to port.

Other input is of course welcome as well.

[SteveSandersonMS]

Not sure if it's of any use to you, but there's a similar-ish thing at we did for Mobile Blazor Bindings, i.e., a .NET implementation of the renderer backend (i.e., what's normally implemented in TypeScript for the browser backend). For example you might recognize the kind of logic at https://github.com/dotnet/MobileBlazorBindings/blob/main/src/Microsoft.MobileBlazorBindings.Core/NativeComponentAdapter.cs#L54

You've probably already got your own version of this logic already but letting you know just in case!

[egil]

Not sure if it's of any use to you, but there's a similar-ish thing at we did for Mobile Blazor Bindings, i.e., a .NET implementation of the renderer backend (i.e., what's normally implemented in TypeScript for the browser backend). For example you might recognize the kind of logic at https://github.com/dotnet/MobileBlazorBindings/blob/main/src/Microsoft.MobileBlazorBindings.Core/NativeComponentAdapter.cs#L54

You've probably already got your own version of this logic already but letting you know just in case!

@SteveSandersonMS my first attempt was actually to import and reuse the MBB project (core parts). However, it make assumptions and simplifications that wont work in this context, as far as I can tell, such as the render tree producing a legal XML/XAML document and not a HTML5 fragment. E.g. it seems that MBB design only allows for an adapter (component?) to only render one element (https://github.com/dotnet/MobileBlazorBindings/blob/main/src/Microsoft.MobileBlazorBindings.Core/NativeComponentAdapter.cs#L273). Perhaps I am misunderstanding the design and using MBB wrong, but did not manage to get it working with HTML5 fragments.

With the current port of BrowserRenderer.ts and ComponentRenderingTestBase.cs tests, code coverage is good for our code. We are missing coverage of the EditType.permutationListEntry and EditType.permutationListEnd related code.

[ScarletKuro]

Side effect on DOM on event handler trigger

This is what I'm waiting for. We all know that Blazor rerenders after handling events (dotnet/aspnetcore#17169 (comment)).
Unless you override this behavior with IHandleEvent.
I'm expecting that the new renderer will work correctly for both cases. When you do not use EventUtil.AsNonRenderingEventHandler and when you use it. This would help MudBlazor to test some scenarios better.

[egil]

This has hung around for long enough. Branch is still there if we want to start this up again.