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

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

Closed
springy76 opened this issue Jun 9, 2022 · 20 comments
Labels
enhancement New feature or request good first issue Good for newcomers input needed When an issue requires input or suggestions needs design More design is needed before this issue should result in a feature being implemented.
Milestone

Comments

@springy76
Copy link

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
Copy link
Member

egil commented Jun 13, 2022

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
Copy link
Author

springy76 commented Jun 14, 2022

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 for example MouseEventArgs where I want to access/log the Details property.

@egil
Copy link
Member

egil commented Jun 14, 2022

@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?

@egil egil added enhancement New feature or request input needed When an issue requires input or suggestions labels Jun 14, 2022
@springy76
Copy link
Author

springy76 commented Jun 14, 2022

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
Copy link
Member

egil commented Jun 14, 2022

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
Copy link
Author

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
Copy link
Sponsor Collaborator

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
Copy link
Member

egil commented Jun 14, 2022

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
Copy link
Author

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
Copy link
Member

egil commented Jun 20, 2022

@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
Copy link
Sponsor Collaborator

@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 egil added good first issue Good for newcomers needs design More design is needed before this issue should result in a feature being implemented. labels Jun 20, 2022
@egil
Copy link
Member

egil commented Jun 20, 2022

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

@Qwertyluk
Copy link

Qwertyluk commented Apr 16, 2024

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
Copy link

Do you have any thoughts?
@egil @linkdotnet

@linkdotnet
Copy link
Sponsor Collaborator

@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
Copy link
Member

egil commented Apr 29, 2024

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
Copy link

Qwertyluk commented Apr 29, 2024

@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
Copy link
Member

egil commented Apr 30, 2024

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

@Qwertyluk
Copy link

I think this issue is resolved now that the #1467 has been merged

@linkdotnet
Copy link
Sponsor Collaborator

Oh yeah it is. Thanks

@linkdotnet linkdotnet added this to the 2.0.0 milestone Jul 4, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request good first issue Good for newcomers input needed When an issue requires input or suggestions needs design More design is needed before this issue should result in a feature being implemented.
Projects
None yet
Development

No branches or pull requests

4 participants