ComponentParameterCollectionBuilder.Add() for EventCallbacks doesn't support async callbacks with input

[springy76]

While there are several overloads of the Add() method having Func<Task> callback parameter, the ones accepting Func<object, Task> callback or Func<TValue, Task> callback are just missing - the non-async versions using Action<object> callback and Action<TValue> callback exist, though.

[egil]

Hi @springy76, can you share a bit more about your use case? In what scenario do you need to perform an async operation on an event callback in your test code?

[springy76]

I'm testing some basic components like async-loading data visualizers and buttons (executing async code) which must be absolutely non-reentrant even when using Blazor-server.

There already is async support, but none of the method transports TValue of EventCallback<TValue> for example MouseEventArgs where I want to access/log the Details property.

[egil]

@springy76, ok, sounds reasonable on the surface. Can you provide a simple example with a test and a component. Would like to make sure we understand your use case correctly?

[springy76]

This extension method - copied from similar Add() method in ComponentParameterCollectionBuilder<TComponent> itself - solved my problem:

public static class ExtensionsMethods
{
	public static ComponentParameterCollectionBuilder<TComponent> Add<TComponent, TValue>(this ComponentParameterCollectionBuilder<TComponent> builder, Expression<Func<TComponent, EventCallback<TValue>>> parameterSelector, Func<TValue, Task> callback)
		where TComponent : IComponent
	{
		return builder.Add(parameterSelector, EventCallback.Factory.Create(callback.Target, callback));
	}
}

[egil]

Thanks @springy76. I would still like to see a test where you use it. We can certainly add a bunch more overloads, just want to make sure the I have use cases for them.

[springy76]

In short/abstracts I both need the values the EventCallbacks receive from the component in order to apply assertions on them, and also simulate fake work by returning Task.Delay() (real services used by the real event handlers would call REST or SQL services) so I can validate the component is rendering proper in-progress UI and also denies concurrent attemps of starting the same work.

Comparing with EventCallbackFactory.Create() overloads also only the 2 additional overloads mentioned in OP are missing.

My sketched use case as code (MouseEventArgs is a bad sample, other EventCallback values contain domain specific data of the app):

var cut = ctx.RenderComponent<RobustIconButton>(pb => pb
	.Add(c => c.OnClick,
		(MouseEventArgs e) =>
		{
			tracker.Track(e);
			return Task.Delay(500);
		})
	);

[linkdotnet]

Maybe just my 2 cents to the whole topic, regardless whether or not the new API is introduced.

Imagine you have the following component:

<button @onclick="TriggerEventWithData">ClickMe</button>
@code {
  [Parameter] public EventCallback<string> OnClick { get; set; }

  private async Task TriggerEventWithData() => await OnClick.InvokeAsync("some payload string");
}

This is testable as of today:

string fromComponent = null;
var cut = context.RenderComponent<MyComponent>(c => c.Add(p => p.OnClick, c => fromComponent = c));

cut.Find("button").Click();

fromComponent.ShouldBe("some payload string");

Now also consider: Do you really want the behavior your posted before? You are kind of bound to the structure and don't test necessary observable behavior. Changes in your DTO or data you are passing around can make your test invalid, render the use-case of the test itself useless.

What I can imagine where you would need that is when a Child-Component should trigger the event and you want to test the behavior. Still here: Not sure if this is something someone wants. Even if so I guess going with the ComponentFactories and creating a stub might be a better way.

[egil]

I think I see the use case here. It's similar to having an async operation in a life cycle method like OnInitializedAsync, where you e.g. render a loading text, then load data from a service, and then display the data.

In theory with async validation, you might want to first show a validating text, then get the data from an async service to do the validation, and then do the validation/show the validation result.

If you want to test you are showing the correct "validating text", you need control over the task.

Does this match your use case @springy76?

[springy76]

It's not about UI/input/form validation but validation=unit-testing (=asserting) that some events transport the right data (not string specific) - and yes, the situation is comparable to OnInitializedAsync where intermediary renders happen while async code is still executing.

[egil]

@linkdotnet I think this is safe for us to add. It is basically a bunch of additional Add methinds in ComponentParameterCollectionBuilder. Do you have any objections or concerns?

[linkdotnet]

@linkdotnet I think this is safe for us to add. It is basically a bunch of additional Add methinds in ComponentParameterCollectionBuilder. Do you have any objections or concerns?

Absolutely fine. Go for it ;)

[egil]

Yes. I will leave this here as "up for grabs" for now.

[Qwertyluk]

I've started working on this issue, however, I've discovered that it may be impossible to expand the ComponmentParameterCollectionBuilder API without causing a breaking change in the compilation process. If overloads that accepts Func<object, Task> or Func<TValue, Task> are introduced, and the client in his code passes null as a parameter to the already existed method that accepts Func<Task>, the compiler will be unable to determine the appropriate overload to invoke. This could lead to the following compilation error:

Error CS0121 The call is ambiguous between the following methods or properties: 'ComponentParameterCollectionBuilder<TComponent>.Add(Expression<Func<TComponent, EventCallback>>, Func<Task>)' and 'ComponentParameterCollectionBuilder<TComponent>.Add(Expression<Func<TComponent, EventCallback>>, Func<object, Task>)' bunit.core.tests (net5.0), bunit.core.tests (net6.0), bunit.core.tests (net8.0), bunit.core.tests (netcoreapp3.1) C:\Projects\bUnit\tests\bunit.core.tests\ComponentParameterCollectionBuilderTests.cs 117 Active

Although it is unlikely that a null value will be passed to the ComponentParameterCollectionBuilder.Add() method as this triggers ArgumentNullException, it can't be assured that no one is catching this exception.

What are your thoughts? Can these overloads be introduced, even if it may break someone's compilation process?

@egil @linkdotnet

[Qwertyluk]

Do you have any thoughts? @egil @linkdotnet

[linkdotnet]

@Qwertyluk, sorry for the late answer; that really went below my radar.

I believe that all parameters are non-nullable, so while this might cause an issue, I think it is fine as it mainly affects our own tests (which explicitly pass in a null!).

[egil]

Yeah sorry about the lack of response. We don't want breaking changes from an end user point of view. If they can install a 1.x+1 version and their code still compiles that's good enough for us. It does not have to be binary compatible.

[Qwertyluk]

@egil If I've understood your reponse correctly, the proposed changes can't be implemented in version 1.x because they might break someone's compilcation process if they pass an explicit null value as a parameter to the existing method (compilator wouldn't know which overloaded version of the method to use).

I've heard that you are developing version 2.x of bUnit. Can these changes be introduced in that version? If so, do you have any guideline for contribution for version 2.x of bUnit?

[egil]

No different than contributing to main. Just checkout v2 and target that with your PR.