#361: Allow untyped JSInterop.Setup methods, that matches with any type

[jgoday]

Pull request description

Fixes #361.

It defines new non-generic Setup methods on BunitJSInteropSetupExtensions, that returns an UntypedJSRuntimeInvocationHandler, who is responsible to create typed JSRuntimeInvocationHandler (JSRuntimeInvocationHandlerFactory) from factory functions (SetResult/SetException/SetCanceled).

Factory functions are defined as Func<JSRuntimeInvocation, T>.

A review of UntypedJSruntimeInvocationHandler/JSRuntimeInvocationHandlerFactory correct names would be necessary.

• Would it be correct to create a JSRuntimeFactoryFunction delegate instead of 'Func<JSruntimeInvocation, T> ?

public delegate T JSRuntimeFunctionFactory<T>(JSRuntimeInvocation invocation);

UntypedJSruntimeInvocationHandler is created from the non-generic setup extension method,
and allow us to return a typed handler from a factory function.

JSRuntimeInvocationHandlerFactory is based on JSRuntimeInvocationHandler,
is created from UntypedJSRuntimeInvocationHandler.SetResult(FactoryFunction) or UntypedJSRuntimeInvocationHandler.SetException(FactoryFunction),
his only responsibility is handling a js invocation and set the specific result or exception on the base class.

PR meta checklist

• Pull request is targeted at main branch for code
  or targeted at stable branch for documentation that is live on bunit.dev.
• Pull request is linked to all related issues, if any.
• I have read the CONTRIBUTING.md document.

Code PR specific checklist

• My code follows the code style of this project and AspNetCore coding guidelines.
• My change requires a change to the documentation.
  • I have updated the documentation accordingly.
• I have updated the appropriate sub section in the CHANGELOG.md.
• I have added, updated or removed tests to according to my changes.
  • All tests passed.

[egil]

Thanks @jgoday. I hope I can get some time tonight or tomorrow between all the moving to look at this.

[egil]

So sorry @jgoday. We're very busy packing up the house so it looks like I wont have time to look at this before some time the weekend.

[jgoday]

So sorry @jgoday. We're very busy packing up the house so it looks like I wont have time to look at this before some time the weekend.

There's no hurry :)

[egil]

Finally had some time to look through the code. There is a scenario that the current solution does not support:

[Fact(DisplayName = "Untyped supports setting result after invocation")]
public async Task Test062()
{
	var sut = CreateSut(JSRuntimeMode.Strict);
	var identifier = "func";
	var jsRuntime = sut.JSRuntime;

	var handler = sut.Setup(i => i.Identifier == identifier);

	var i1 = jsRuntime.InvokeAsync<bool>(identifier);

	handler.SetResult(_ => false);

	(await i1).ShouldBe(false);
}

Here, the SetResult method is called after the jsRuntime.InvokeAsync, which is a scenario we need to support. However, the current solution fails with:

    Bunit.JSRuntimeUnhandledInvocationException : bUnit's JSInterop has not been configured to handle the call:
    
        InvokeAsync<bool>("func")
    
    Configure bUnit's JSInterop to handle the call with following:
    
        Setup<bool>("func")
    
    The setup methods are available on an instance of the BunitJSInterop or
    BunitJSModuleInterop type. The standard BunitJSInterop is available
    through the TestContext.JSInterop property, and a BunitJSModuleInterop
    instance is returned from calling SetupModule on a BunitJSInterop instance.
    

  Stack Trace: 
    BunitJSInterop.TryHandlePlannedInvocation[TValue](JSRuntimeInvocation invocation) line 87
    BunitJSInterop.HandleInvocation[TValue](JSRuntimeInvocation invocation) line 69
    JSRuntimeExtensions.HandleInvokeAsync[TValue](BunitJSInterop jSInterop, String identifier, Object[] args) line 14
    BunitJSRuntime.InvokeAsync[TValue](String identifier, Object[] args) line 23
    JSRuntimeExtensions.InvokeAsync[TValue](IJSRuntime jsRuntime, String identifier, Object[] args)
    BunitJSInteropTest.Test062() line 568
    --- End of stack trace from previous location where exception was thrown ---

[jgoday]

Finally had some time to look through the code. There is a scenario that the current solution does not support:

[Fact(DisplayName = "Untyped supports setting result after invocation")]
public async Task Test062()
{
	var sut = CreateSut(JSRuntimeMode.Strict);
	var identifier = "func";
	var jsRuntime = sut.JSRuntime;

	var handler = sut.Setup(i => i.Identifier == identifier);

	var i1 = jsRuntime.InvokeAsync<bool>(identifier);

	handler.SetResult(_ => false);

	(await i1).ShouldBe(false);
}

Here, the SetResult method is called after the jsRuntime.InvokeAsync, which is a scenario we need to support. However, the current solution fails with:

    Bunit.JSRuntimeUnhandledInvocationException : bUnit's JSInterop has not been configured to handle the call:
    
        InvokeAsync<bool>("func")
    
    Configure bUnit's JSInterop to handle the call with following:
    
        Setup<bool>("func")
    
    The setup methods are available on an instance of the BunitJSInterop or
    BunitJSModuleInterop type. The standard BunitJSInterop is available
    through the TestContext.JSInterop property, and a BunitJSModuleInterop
    instance is returned from calling SetupModule on a BunitJSInterop instance.
    

  Stack Trace: 
    BunitJSInterop.TryHandlePlannedInvocation[TValue](JSRuntimeInvocation invocation) line 87
    BunitJSInterop.HandleInvocation[TValue](JSRuntimeInvocation invocation) line 69
    JSRuntimeExtensions.HandleInvokeAsync[TValue](BunitJSInterop jSInterop, String identifier, Object[] args) line 14
    BunitJSRuntime.InvokeAsync[TValue](String identifier, Object[] args) line 23
    JSRuntimeExtensions.InvokeAsync[TValue](IJSRuntime jsRuntime, String identifier, Object[] args)
    BunitJSInteropTest.Test062() line 568
    --- End of stack trace from previous location where exception was thrown ---

Added last example to tests (Test062), It should work now.

UntypedJSRuntimeInvocationHandler :

• Behave as a generic handler for all invocations, when the result/exception/cancellation is set after the invocation, it runs the factory function against it.
  In this case, the handler does not know the current return type until setResult/setException... so, it only completes when the factory function returns the correct type, if not, BUnitJSInterop will try to invoke the other possible handlers.

• When the result/exception/cancellation is called before the invocation, UntypedJSRuntimeInvocationHandler register specific handlers for the correct types and removes itself from the invocations chain.

Is this a valid solution ?

[egil]

Hmm I am starting to get cold feet on this one tbh. What troubles me is that it should be possible to add/register one or more responses, each matching different invocations and return types, right?

Or maybe it should just be possible to provide a single Func<invocation, task<T>> resultFactory to SetResult/SetExeption, and so on. But then the user would have to provide all the basic implementation that is in the base handler, so maybe we just need a way to add a custom handler for the few cases that has this requirements.

Does this make sense?

[jgoday]

Hi @egil,
Before continuing with the code, I will try to show/clarify the idea of how
the implementation might be and let's discuss the possible errors/downsides.

Notice that, probably, I am not fully aware of the scope of the problem.

Here are the possible steps:

1. Define an interface IJSRuntimeInvocationHandler. It's only purpose is to define how a js runtime invocation handler should be, with no type information.

       public interface IJSRuntimeInvocationHandler
       {
           Task<TResult> HandleAsync<TResult>(JSRuntimeInvocation invocation);
           bool CanHandle(JSRuntimeInvocation invocation);
       }

   Traditional JSRuntimeInvocationBase implements IJSRuntimeInvocationHandler,
   its behavior is not affected anyway.

2. A base implementation (UntypedJSRuntimeInvocationHandler) that implements IJSRuntimeInvocationHandler. It can register as many factory functions as the user wants. This handler is returned using
   the Setup extension method without any type parameter.

       Setup(i => i.Identifier == "foo").SetResult(_ => true);
       Setup(i => i.Identifier == "bar").SetResult(_ => 1);
   
       var t1 = jsRuntime.InvokeAsync<bool>("foo");
       var t2 = jsRuntime.InvokeAsync<int>("bar");
       
       (await t1).ShouldBe(true);
       (await t2).ShouldBe(1);

3. Results can be set after invocation too:

       var h1 = Setup(i => i.Identifier == "foo");
       var h2 = Setup(i => i.Identifier == "bar");
   
       var t1 = jsRuntime.InvokeAsync<bool>("foo");
       var t2 = jsRuntime.InvokeAsync<int>("bar");
   
       h1.SetResult(_ => true);
       h2.SetResult(_ => 1);
   
       (await t1).ShouldBe(true);
       (await t2).ShouldBe(1);

4. BUnitJsInterop keeps the handlers in a Dicionary<TResult, List>, just as the actual code.

   • When registering a handler as IJsRuntimeInvocationHandler (no result type information),
     it adds it to the handlers dictionary with the 'AnyType' Type

   internal static class AnyType {}

   • BUnitJsInterop.TryHandlePlannedInvocation:
     • It looks for an IJSRuntimeInvocationHandler (could be a JSRuntimeInvocationBase or an untyped impl like UntypedJSRuntimeInvocationHandler):

     if (TryGetHandlerFor<TValue>(invocation) is IJSRuntimeInvocationHandler handler)
     {
         var task = handler.HandleAsync<TValue>(invocation);
         result = new ValueTask<TValue>(task);
     }

   • BUnitJsInterop.TryGetHandlerFor
     • Changes the signature to (only changes the return type)

     internal IJSRuntimeInvocationHandler? TryGetHandlerFor<TResult>(
         JSRuntimeInvocation invocation, 
         Predicate<JSRuntimeInvocationHandlerBase<TResult>>? handlerPredicate = null)

     • Internally, it looks first for an IJSRuntimeInvocationHandler
       and then (if no IJSRuntimeInvocationHandler is not found), searchs for a handler (JSRuntimeInvocationHandlerBase) by its return type

5. Later on, JSRuntimeInvocationHandlerBase can be implemented extending over UntypedJSRuntimeInvocationHandler

[egil]

Let me elaborate a bit more on my general concerns (was on a phone last time, so was keystroke limited).

Supposed we set up a untyped handler for every invocation whose identifier starts with myJsLib:

var handler = Setup(i => i.Identifier,StartsWith("myJsLib"));

Its not a completely unreasonable scenario, since you might want to handle all calls to a js lib with the following functions:

myJsLib.getText() : string
myJsLib.getAnotherText() : string
myJsLib.getNum() : number

How should the API for specifying the return type (or exception) look?

handler.SetResult(inv => 
{
  if(inv.Identifier.EndsWith("getText")
    return "foo";
  if(inv.Identifier.EndsWith("getAnotherText")
    return "bar";
  if(inv.Identifier.EndsWith("getNum")
    return 42;
});

This wont work unless SetResult takes a Func<Invocation, object> as input, since different result types must be supported, as far as I can tell.

If we instead just allow the handler to receive multiple calls:

handler.SetResult<string>(inv =>
{
  if(inv.Identifier.EndsWith("getText")
    return "foo";
  if(inv.Identifier.EndsWith("getAnotherText")
    return "bar";
  return ???;
})
handler.SetResult<int>(inv => 42);

This looks a bit weird to me. Also, what if I want e.g. getAnotherText to return an exception, or not return at all, how should that look?

What I think the key is, is that there is a duality between Setup and SetXXXX calls, and they cannot have different number of branches, e.g. one setup call that matches 10 JS functions, but only 5 SetXXXX calls specifying results. If there is, its going to lead to too much "magic" for the end-user.

The original use case was to make it easy for 3rd party component vendors to do a one liner that would set up handling for all their js calls, so that users of the 3rd party component lib could write a test of a component that uses a component from the 3rd party lib without having to worry about complex jsinterop setup logic. However, with the above in mind, its pretty clear that a one liner would be pretty hard to achieve in many cases.

What do you think?

[jgoday]

Let me elaborate a bit more on my general concerns (was on a phone last time, so was keystroke limited).

Supposed we set up a untyped handler for every invocation whose identifier starts with myJsLib:

var handler = Setup(i => i.Identifier,StartsWith("myJsLib"));

Its not a completely unreasonable scenario, since you might want to handle all calls to a js lib with the following functions:

myJsLib.getText() : string
myJsLib.getAnotherText() : string
myJsLib.getNum() : number

How should the API for specifying the return type (or exception) look?

handler.SetResult(inv => 
{
  if(inv.Identifier.EndsWith("getText")
    return "foo";
  if(inv.Identifier.EndsWith("getAnotherText")
    return "bar";
  if(inv.Identifier.EndsWith("getNum")
    return 42;
});

This wont work unless SetResult takes a Func<Invocation, object> as input, since different result types must be supported, as far as I can tell.

If we instead just allow the handler to receive multiple calls:

handler.SetResult<string>(inv =>
{
  if(inv.Identifier.EndsWith("getText")
    return "foo";
  if(inv.Identifier.EndsWith("getAnotherText")
    return "bar";
  return ???;
})
handler.SetResult<int>(inv => 42);

This looks a bit weird to me. Also, what if I want e.g. getAnotherText to return an exception, or not return at all, how should that look?

What I think the key is, is that there is a duality between Setup and SetXXXX calls, and they cannot have different number of branches, e.g. one setup call that matches 10 JS functions, but only 5 SetXXXX calls specifying results. If there is, its going to lead to too much "magic" for the end-user.

The original use case was to make it easy for 3rd party component vendors to do a one liner that would set up handling for all their js calls, so that users of the 3rd party component lib could write a test of a component that uses a component from the 3rd party lib without having to worry about complex jsinterop setup logic. However, with the above in mind, its pretty clear that a one liner would be pretty hard to achieve in many cases.

What do you think?

Thank you @egil :) Sorry for the late response, I will try to look and work on this starting the week.

[egil]

I look forward to seeing your solution.

Ps. Going camping next week, so might not be able to review your code before after that.

[egil]

hey @jgoday, im going to close this PR. If you want to continue with this, just reopen it/resubmit it.

[jgoday]

hey @jgoday, im going to close this PR. If you want to continue with this, just reopen it/resubmit it.

Ok @egil, thank you for you time and really sorry for not been able to finish it (I cannot focus on this right now)

[egil]

No worries. I understand!