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

Strongly typed/type safe passing parameters to components during render and rerender #79

Closed
egil opened this issue Mar 29, 2020 · 46 comments · Fixed by #94
Closed

Strongly typed/type safe passing parameters to components during render and rerender #79

egil opened this issue Mar 29, 2020 · 46 comments · Fixed by #94
Assignees
Labels
enhancement New feature or request help wanted Extra attention is needed input needed When an issue requires input or suggestions
Milestone

Comments

@egil
Copy link
Member

egil commented Mar 29, 2020

Using c# expressions trees to pass parameters to a component during render could make it strongly typed and type safe, avoiding the use of the nameof operator, etc.

The basic idea looks like this:

// MyComponent.razor
@code 
{
    [Parameter] public string Name { get; set; }
    [Parameter] public int Age { get; set; }
}

Test code:

var cut = RenderComponent<MyComponent>(c => c.Name = "asdf", c => c.Age = 42);

I think the signature of RenderComponent then should look like the following... haven't played with expression trees like this before:

T RenderComponent<T>(params Expression<Action<T>>[] parameters)

The RenderComponent method would then translate the expression into a ParameterView with the names and values specified in the lambda.

It would be nice if it was possible to only have to pass one lambda and assign all parameters inside that.

Is this possible? Is it better than the current way to pass parameter?

Input definitely wanted!

@egil egil added enhancement New feature or request help wanted Extra attention is needed input needed When an issue requires input or suggestions labels Mar 29, 2020
@StefH
Copy link
Contributor

StefH commented Mar 29, 2020

Example code (not tested):

New method which accepts c => c.Name == "x", c => c.Age == 42

public IRenderedComponent<TComponent> RenderComponent<TComponent>(params Expression<Func<TComponent, object>>[] parameters) where TComponent : class, IComponent
    {
        var componentParameters = new List<ComponentParameter>();
        foreach (var parameter in parameters)
        {
            if (parameter.Body is UnaryExpression body)
            {
                if (body.Operand is BinaryExpression methodBinaryExpression)
                {
                    if (methodBinaryExpression.Left is MemberExpression left && methodBinaryExpression.Right is ConstantExpression right)
                    {
                        var name = left.Member.Name;
                        var value = right.Value;

                        componentParameters.Add(ComponentParameter.CreateParameter(name, value));
                    }
                }
            }
        }

        return base.RenderComponent<TComponent>(componentParameters.ToArray());
    }

Call it like:

var cut = x.RenderComponent<Com>(c => c.Name == "x", c => c.Age == 42);

Gist:
https://gist.github.com/StefH/6229f0f264aeb68b567e5bf90b960922

@egil
Copy link
Member Author

egil commented Mar 29, 2020

Nice @StefH. What do you think about that syntax vs. the current one?

@StefH
Copy link
Contributor

StefH commented Mar 29, 2020

Both are fine. (I think?)
The original one can be used if some flexibility is needed, the new one is more restricting.

BTW : I don't know if the original method supports nesting?
Like

ComponentParameter.CreateParameter("Parent.Age", 99);

Because my example can also not yet handle nested objects...

@egil
Copy link
Member Author

egil commented Mar 29, 2020

Blazor doesn't allow you to set nested properties on parameters natively afaik, so there is no need.

Personally I like it better so far. Nameof is currently required to get something just of semi refactor safe, but types are not checked even with that. This has the advantages of being safe when refactoring a parameter, and if you change the type of the parameter, the compiler will let you know what tests to fix. That will help avoid weird issues and possible false positives. I also think it will help make it easier for intellisense to suggest the right things.

What is more interesting is the more advanced parameter types like EventCallbacks, RenderFragment, and RenderFragment.

Got a good suggestion for dealing with them?

@StefH
Copy link
Contributor

StefH commented Mar 30, 2020

About EventCallbacks, RenderFragment and RenderFragment : can you point me to the code or unit-test how they are used now?

@egil
Copy link
Member Author

egil commented Mar 30, 2020

Yes of course. The current syntax is described here: https://bunit.egilhansen.com/docs/csharp-based-testing.html#passing-parameters-to-components-during-render

There is an additional Template method, that I have not documented yet it seems: https://github.com/egil/bunit/blob/master/src/ComponentTestFixture.cs#L250 - It abstracts away the render tree builder logic for a more simple func based builder.

@StefH
Copy link
Contributor

StefH commented Mar 30, 2020

I think the example code I provided will not work for events.

Another possible interfaces could be:

// With a sort of builder-pattern
var r2 = x.RenderComponent2<Com>(cb =>
{
    var c = new Com
    {
        Age = 42,
        Name = "n",
        NonGenericCallback = EventCallback.Empty
    };

    cb.Set(c);
});

// Just provide a new instance from the component and copy these values to parameters
var r3 = x.RenderComponent3(new Com
{
    Age = 42,
    Name = "n",
    NonGenericCallback = EventCallback.Empty
});

// Just provide an action to set some values to a new component and copy these values to parameters
var r4 = x.RenderComponent4<Com>(c =>
{
    c.Name = "x";
    c.Age = 42;
    c.NonGenericCallback = EventCallback.Empty;
    c.GenericCallback = new EventCallback<EventArgs>();
});

See https://github.com/StefH/BUnitExamples/blob/master/ConsoleAppBUnit/Program.cs

@egil
Copy link
Member Author

egil commented Mar 30, 2020

Ok, thanks. I will experiment a little when I am done with the split lib issues. If you want ot submit a PR or continue to experiment, please do.

@EdCharbeneau
Copy link
Contributor

I like the syntax, expressions are really powerful and pretty well understood. They can get a little hectic to write APIs for, but once you understand them it's easy going. I wish you could use them easier in Razor Components but it doesn't quite work the same as it did with HTML Helpers. There's no way to scope models, etc.

In this scenario I like it.

@StefH
Copy link
Contributor

StefH commented Apr 1, 2020

@EdCharbeneau which syntax do you like best?

  • r2 - With a sort of builder-pattern
  • r3 - Just provide a new instance from the component and copy these values to parameters
  • r4 - Just provide an action to set some values to a new component and copy these values to parameters

@egil
Copy link
Member Author

egil commented Apr 1, 2020

@StefH I am not sure r2 to r4 will work. How would you unset a property/set a property to null?

We need a syntax that allow setting only the specified values, both initially and on re-renders through the SetParametersAndRender method.

The first one where we pass a lambda for each property is still more terse than the current recommended.

@StefH
Copy link
Contributor

StefH commented Apr 1, 2020

What about this:

var builder = new ComponentParameterTypedBuilder<Com>();
builder.Set(c => c.Name, "name");
builder.Set(c => c.Age, 42);

var r5 = x.RenderComponent5(builder);

@egil
Copy link
Member Author

egil commented Apr 1, 2020

What advanced does the r5 syntax have over the one you originally proposed (​var​ ​cut​ ​=​ ​x​.​RenderComponent​<​Com​>(​c​ ​=>​ ​c​.​Name​ ​==​ ​"​x​"​, ​c​ ​=>​ ​c​.​Age​ ​==​ ​42​);). The original seems much more terse to me, and should allow the same scenarios?

@StefH
Copy link
Contributor

StefH commented Apr 1, 2020

That original does not work with Event handlers (c => c.Name == "x", c => c.Age == 42) Because it uses == equal syntax , which is wring.

@StefH
Copy link
Contributor

StefH commented Apr 1, 2020

More examples:

var r6 = x.RenderComponent6<Com>(
                c => c.Name = "stef",
                c => c.Age = 42
            );

var r7 = x.RenderComponent<Com>(
  ComponentParameterTyped<Com>.Create(c => c.Name, "n"),
  ComponentParameterTyped<Com>.Create(c => c.Age, 3)
);

@egil
Copy link
Member Author

egil commented Apr 1, 2020

Why would this not allow us to pass a EventHandler?

// MyComponent.razor
@code 
{
    [Parameter] public string Name { get; set; }
    [Parameter] public int Age { get; set; }
    [Parameter] public EventCallback<int> AgeChanged { get; set; }
}
RenderComponent<MyComponent>(
   x => x.AgeChanged = EventCallback.Factory.Create(this, () => { /* handler code */ });
);

Obviously, the EventCallback factory code is bit verbose, but a helper method could probably help with this.

@StefH
Copy link
Contributor

StefH commented Apr 1, 2020

Hi @egil , sorry for the confusion, the original one cannot be used to provide an eventhandler.

That original does not work with Event handlers (c => c.Name == "x", c => c.Age == 42) Because it uses == equal syntax , which is wrong.

Al other options r2,r3,r4,r5,r6,r7 can work.
Personally I think that r6 is the one you want, however to get this functionally working, some hacking needs to be done ;-)

@egil
Copy link
Member Author

egil commented Apr 2, 2020

Yes, r6 and my latest example is the one.

@EdCharbeneau
Copy link
Contributor

I like 4 a lot.

@egil
Copy link
Member Author

egil commented Apr 2, 2020

I also like r4, it looks like it is the most terse of the bunch, but that would, as far as I can see, require us to instantiate MyComponent, pass it to the Action<MyComponent>, to pull out the assigned values.

Three problems with that:

  • if MyComponent has a constructor that does stuff, e.g. calls a static class and sets something that has a side effect, it could change the result of the test or break it.
  • assigning to a property/parameter might change the value in the setter, or retrieving it again might change it in the getter.
  • detecting if users explicitly assign null to a property will be hard.

The only way I can think of right now to make r4 work would be to use a mocking framework to create a mock of complement under test and use that to capture the values assigned to properties/parameters, including nulls.

@StefH
Copy link
Contributor

StefH commented Apr 2, 2020

About that last remark. I'm investigating multiple mocking frameworks. However to get that working you need to use virtual properties.

About creating a new component: I already thought that would be an issue in case code is defined in the constructor.

@StefH
Copy link
Contributor

StefH commented Apr 3, 2020

I tried several mocking frameworks: FakeItEasy, JustMock, Moq, NSubstitute and all will call the constructor. So that's a no-go if we do not want do a new() and call a constructor.

I did some research for options r4 and r6, and it's somehow possible to implement this, but using Mono.Reflection to decompile the actions in IL instructions and then parse the instructions.
This seems to work for simple assignments (settings constant values, simple integer additions, and simple new statements) like:

var r6 = x.RenderComponent6<Com>(
                c => c.Name = "stef", // simple constant assigment
                c => c.NameNull = null, // simple constant assigment
                c => c.Age = 42 + 1, // simple assigment
                c => c.NonGenericCallback = EventCallback.Empty, // simple assigment
                c => c.GenericCallback = new EventCallback<EventArgs>() // simple new() statement
            );

However some complex like below does not yet work easily:

string n = "name";
var r6 = x.RenderComponent6<Com>(
                c => c.Name = "stef" + name, // this is another method call
            );

And code like EventCallback.Factory.Create(this, () => { /* handler code */ }); will also not work yet.

Just some thoughts....

@egil
Copy link
Member Author

egil commented Apr 3, 2020

Ok, thanks. And expression tree's doesn't work with assignment (=), only with comparison (==)?

Update: It seems that expression trees does support assignment, but the c# compiler doesn't. Too bad.

@egil
Copy link
Member Author

egil commented Apr 3, 2020

It does seem that JustMock supports mocking of concrete classes without virtual properties and not calling the constructor, but it is in the paid version: https://docs.telerik.com/devtools/justmock/advanced-usage/concrete-mocking#concrete-classes-advanced-mocking

OK that looks like it requires users to have JustMock installed as a plugin, so that is not a feasible approach for us,

@StefH
Copy link
Contributor

StefH commented Apr 4, 2020

Option r8 is using Tuples:

var r8 = x.RenderComponent8<Com>(
                (c => c.Name, "d"),
                (c => c.Age, 3)
);

The only thing is that I needed to add some type checking and throw an exception if they dont match.

Because this interface allows also:

 (c => c.Name, 1234),

because there is not way I can use generic/typed Tuples in a params array.

@egil
Copy link
Member Author

egil commented Apr 4, 2020

Interesting attempt. It seems we are now full circle back to the current implementation, at least from a type safety point of view.

When looking at the rendered code from the Blazor compiler, we can see that they use a runtime type checker, so I wonder if the reason we have not found a solution is because there is none?

There is another, related issue, #36, it might be that it's the approach we need to investigate... Create a component parameter builder.

@StefH
Copy link
Contributor

StefH commented Apr 4, 2020

ParameterBuilder, that was my idea r5 indeed, and that one is typed.

var builder = new ComponentParameterTypedBuilder<Com>();
builder.Set(c => c.Name, "name");
builder.Set(c => c.Age, 42);

var r5 = x.RenderComponent5(builder);

@egil
Copy link
Member Author

egil commented Apr 4, 2020

Yep. Lets investigate that syntax a little. Would something like this be possible:

RenderComponent<T>(Action<ComponentParameterBuilder<T>> builder);

var cut = RenderComponent<Comp>(b => 
    b.Set(c => c.Name, "foo")
     .Set(c => c.Xxx, new Xxx())
);

Should it be Add instead of Set for the method calls?

(typing on my phone, not sure if this compiles)

Ps. I really appreciate all your work and help on this. It is super nice to have somebody to brainstorm and experiment with.

@StefH
Copy link
Contributor

StefH commented Apr 4, 2020

That's possible:

            var r9 = x.RenderComponent9<Com>(builder => builder
                .Add(c => c.Name, "n")
                .Add(c => c.Age, 3)
            );

@egil
Copy link
Member Author

egil commented Apr 4, 2020

R9 might be the best compromise. It even allows us to have addition specialized overloads such as AddChildContent(string html) or AddChildContent<TChildComponent>(Action<ComponentParameterBuilder> childCompParamBuilder). Similar tricks for CascadingValues.

For other param types that require a name, simple overloads of Add will work nicely (e.g. EventCallbacks). This also gives us a generalized way to pass parameters that is not super tied to a specific testing framework (#5).

@StefH
Copy link
Contributor

StefH commented Apr 4, 2020

I'll make a PR, so you can look how it can work.

@egil
Copy link
Member Author

egil commented Apr 4, 2020

Awesome. Look at https://github.com/egil/bunit/blob/master/src/ComponentTestFixture.cs for inspiration for useful overloads.

Please add tests as well if you have the time.

@StefH
Copy link
Contributor

StefH commented Apr 4, 2020

#92

@egil egil added this to the beta-7 milestone Apr 4, 2020
@StefH
Copy link
Contributor

StefH commented Apr 4, 2020

#94

@StefH
Copy link
Contributor

StefH commented Apr 5, 2020

@egil
I made an attempt to make the ComponentParameter generic, and I think it is possible. See my pull request #94 for the code. Just have a look to see if this would also be an solution (besides the builder)

Also a question:
Why is one method called CreateParameter(), and the other CreateCascadingValue ?
Shouldn't it just be:

  1. Create and CreateCascading
    or
  2. CreateValue and CreateCascadingValue
    or
  3. CreateParameter and CreateCascadingParameter
    or
  4. CreateParameterValue and CreateCascadingParameterValue

@egil egil linked a pull request Apr 5, 2020 that will close this issue
@egil
Copy link
Member Author

egil commented Apr 5, 2020

@StefH, I will take a look tomorrow when the kids are out of my hair. Thanks you for your work on this!

@egil
Copy link
Member Author

egil commented Apr 6, 2020

@StefH I have added a first review of the code in the PR. Lets discuss code specific things in the PR thread.

Also a question:
Why is one method called CreateParameter(), and the other CreateCascadingValue ?
Shouldn't it just be:

  1. Create and CreateCascading
    or
  2. CreateValue and CreateCascadingValue
    or
  3. CreateParameter and CreateCascadingParameter
    or
  4. CreateParameterValue and CreateCascadingParameterValue

You have a point. Best explanation is that naming is hard :) If we should change things around, then I like the 3. option best.

@egil egil closed this as completed Apr 15, 2020
@egil
Copy link
Member Author

egil commented May 8, 2020

Hey @StefH

I am playing around with this, trying to convert some of the built in test to the new syntax, and I am not sure I am finding it as intuitive and easy to read as I have hoped for. E.g.:

var wrapper = RenderComponent<TwoComponentWrapper>(
    RenderFragment<Wrapper>(nameof(TwoComponentWrapper.First),
        ChildContent<Simple1>((nameof(Simple1.Header), "First")
    )),
    RenderFragment<Simple1>(nameof(TwoComponentWrapper.Second), (nameof(Simple1.Header), "Second"))
);

var wrapper = RenderComponent<TwoComponentWrapper>(builder => builder
    .Add<Wrapper>(p => p.First, wrapper => wrapper
        .AddChildContent<Simple1>(simple1 => simple1
            .Add(p => p.Header, "First")))
    .Add<Simple1>(p => p.Second, simple1 => simple1
        .Add(p => p.Header, "Second"))
);

I my example here I have tried have played around with indention and naming of the sub builders.

What do you think, any suggestions for how to make this cleaner?

@StefH
Copy link
Contributor

StefH commented May 10, 2020

I see. However these examples are using child-content. For normal variables this looks better I think.

@egil
Copy link
Member Author

egil commented May 10, 2020

Absolutely, and I generally like the builder syntax, just looking for ideas to improve the hard cases.

@egil egil reopened this May 17, 2020
@egil
Copy link
Member Author

egil commented May 17, 2020

Reopening this as there is a case we have to consider. Users might want to provide a cascading value to a complement inside the TComponent. I believe this is not possible.

Best cause of action I can think of right now is to create a special AddCascadingValue(string name, object value) method to handle the situation, and in addition rename the Add(object value) method to AddCascadingValue(object value) for consistency. Perhaps AddCascading instead of AddCascadingValue.

An alternative is to provide a general way to specify that TComponent should be wrapped by one or more components, that should be rendered before it in the render tree. That way a CascadingValue component could be added that way, and other components could also be added. This is sometimes needed when users are using other libs that require a root component to be added, e.g. #131.

I am actually leaning more towards the second alternative here since it would be more general purpose.

@StefH
Copy link
Contributor

StefH commented May 18, 2020

Reopening this as there is a case we have to consider. Users might want to provide a cascading value to a complement inside the TComponent. I believe this is not possible.

You mean "... to a component inside the ..."?

And can you provide a full example or usage scenario how this currently works, and how it should work?

I'm afraid I don't get it exactly what you mean.

@egil
Copy link
Member Author

egil commented May 18, 2020

I was inspired by #135 for this.

Basically, if you have a render tree like this:

<CascadingAuthorizationState> <-- component that should wrap TComponent
    <MyComponentUnderTest> <-- TComponent
        <AuthorizeView> <-- Component with AuthorizationState cascading parameter
            <Authorized>
                 ...

@egil egil modified the milestones: beta-7, beta-8 May 18, 2020
@StefH
Copy link
Contributor

StefH commented May 18, 2020

Could this be the class definitions?

public class CascadingAuthorizationState : ComponentBase
{
	[CascadingParameter] public RenderFragment MyComponent { get; set; }
}

public class MyComponentUnderTest : ComponentBase
{
	[CascadingParameter] public RenderFragment View { get; set; }
}

public class AuthorizeView : ComponentBase
{
	[CascadingParameter] public RenderFragment State { get; set; }
}

public class AuthorizationState : ComponentBase
{
	[Parameter] public string Test { get; set; }
}

@egil
Copy link
Member Author

egil commented May 18, 2020

@StefH not sure I understand. The all the auth related components are part of Blazors core lib, e.g. CascadingAuthenticationState. It looks like the rest are here: https://github.com/dotnet/aspnetcore/tree/master/src/Components/Authorization/src

If you want the core idea to code against, lets simplify it a bit. Suppose we want to pass a ThemeState class down from the root of our render tree in our Blazor app, so all themeable components can ask for the current theme settings:

class ThemeState
{
    public bool IsDarkMode {get;}
}

The provider of ThemeState is:

// CascadingThemeState.razor
<CascadingValue Value="new ThemeState()">
    @ChildContent
</CascadingValue>
@code {
     [Parameter] public RenderFragment ChildContent { get; set; }
}

The receiver of ThemeState is:

// ThemableButton.razor
<button class=@(Theme.IsDarkMode ? "dark-btn" : "light-btn")>CLICK ME</button>
@code {
    [CascadingParameter] public ThemeState Theme {get; set;}
}

The component under test is:

// MyComponent.razor
<h1>Here is a button you can click...</h1>
<ThemableButton/>

And in production code it would be used like this:

<CascadingThemeState>
    <MyComponent />
</CascadingThemeState>

The neat thing about cascading values is that they cascading through the component tree, even if a component, in this case MyComponent, does not have a parameter that captures it.

@egil egil closed this as completed May 18, 2020
@egil egil reopened this May 18, 2020
@egil
Copy link
Member Author

egil commented Jun 22, 2020

After further consideration, I think this is actually solved already. If you want to wrap a component around the CUT, it is definitely possibe, e.g. http://localhost:8080/docs/providing-input/passing-parameters-to-components.html#render-a-component-under-test-inside-other-components

Close this again :)

@egil egil closed this as completed Jun 22, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request help wanted Extra attention is needed input needed When an issue requires input or suggestions
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants