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

Support chained 'bind' on components #5504

Closed
floreseken opened this issue Apr 18, 2018 · 16 comments
Closed

Support chained 'bind' on components #5504

floreseken opened this issue Apr 18, 2018 · 16 comments
Assignees
Labels
area-blazor Includes: Blazor, Razor Components bug This issue describes a behavior which is not expected - a bug.

Comments

@floreseken
Copy link

I must be doing something wrong.

Made this component:

<div>
    <input bind="@myValue" />
</div>

@functions
{
    public string myValue { get; set; }
    public Action<string> myValueChanged { get; set; }
}

And use this in a page like this:

<input bind="@Test" /><br />

<myInput bind-myValue="@Test"></myInput><br />

test: @Test

@functions {

    public string Test { get; set; }
}

Now I expect that typing in either input on this page would result in the same values in both inputs. But this seems to work only one-way: from the input to the control and not the other way around.

This is supposed to work right? What am I doing wrong?

Note (from @SteveSandersonMS)

I expect fixing this will also fix #1490. Any proposed fix for this issue should also be checked to verify it accounts for the scenarios raised at #1490, and if it doesn't we'll reopen #1490 as a separate issue.

@danroth27
Copy link
Member

danroth27 commented Apr 18, 2018

I believe you need to call the myValueChanged action in your myInput component when myValue changes. Presumably you'd want to do that when the input in the myInput component changes. I don't think you can use bind then on the input because I think you can only have one event handler for onchange (bind adds an onchange event handler). But you can set value and implement onchange yourself like this:

<div>
    <input value="@myValue" onchange="@InputChanged" />
</div>

@functions
{
    public string myValue { get; set; }
    public Action<string> myValueChanged { get; set; }

    void InputChanged(UIChangeEventArgs e)
    {
        myValue = (string)e.Value;
        myValueChanged(myValue);
        Console.WriteLine($"InputChanged: {myValue}");
    }
}

However, the myValueChanged event doesn't seem to cause the parent component to render (@rynowak is this a bug?), so currently I think you have to do this in the parent component:

<input bind="@Test" /><br />

<myInput myValue="@Test" myValueChanged="@MyValueChanged" /><br />

test: @Test

@functions {
    public string Test { get; set; }

    void MyValueChanged(string myValue)
    {
        Console.WriteLine($"MyValueChanged: {myValue}");
        Test = myValue;
        StateHasChanged();
    }
}

@floreseken
Copy link
Author

Ah, this helps. I now see that the documentation nowhere mentions that databinding is two-way. I assumed it was going to work this way, I guess because Aurelia and Angular do it this way.

Are you planning on making it work this way? I'll opt for that, it looks cumbersome as it is right now.

@SteveSandersonMS
Copy link
Member

@rynowak To make the multiple-connected-bindings scenario work correctly, I propose we amend the codegen output for bind so that:

  1. After the change handler writes the new property value, we invoke StateHasChanged. It doesn't matter if this means there are cases where it gets invoked twice synchronously in succession, because the BlazorComponent base class no-ops additional calls if there's already a queued render. This will fix the problem noted by Dan above.
  2. If you're binding to MyProp, and your component also declares a property Action MyPropChanged { get; }, then after writing to MyProp we should invoke MyPropChanged. This will fix the original issue reported here.

What do you think? Is the second item there technically reasonable to implement - do we know whether a certain-named other property exists at the right point in the compilation process? I'm guessing that the two-phase compile makes this possible, though I don't know whether it would be convenient to do still.

@floreseken Data binding is two way, so I'm not sure what you mean by that comment. There are a couple of usability issues with it, for which I'm proposing the two fixes above.

@rynowak
Copy link
Member

rynowak commented Apr 19, 2018

@SteveSandersonMS - yes, both of these sounds like good enhancements and very doable.

If you're binding to MyProp, and your component also declares a property Action MyPropChanged { get; }, then after writing to MyProp we should invoke MyPropChanged. This will fix the original issue reported here.

FYI If you're in the case where you don't declare MyPropChanged we don't provide automatic completion for bind-MyProp. You can still write it yourself, but we don't add it to the completion list. I still think this is the right behavior, let me know if you have feedback

@SteveSandersonMS
Copy link
Member

@rynowak Sounds right!

@dlr1
Copy link

dlr1 commented Apr 19, 2018

If I have something like bind-myProp-myPropChanged="somevariable" and not actually declare myPropChanged will it be invoked?

@floreseken
Copy link
Author

No, even if you do declare it, it doesn't get invoked magically.

But Steve is proposing a change that will.

@SteveSandersonMS SteveSandersonMS changed the title How to get two-way databinding to work with custom component Support chained 'bind' on components Apr 20, 2018
@floreseken
Copy link
Author

floreseken commented May 9, 2018

@SteveSandersonMS could this please be part of the 0.4 release? It is holding me back from creating any component with inputs in them.

@floreseken
Copy link
Author

I've written a blogpost on a workaround which involves less code if you have a lot of components.

http://flores.eken.nl/blazor-custom-component-with-2-way-databinding/

@vertonghenb
Copy link
Contributor

Maybe something for 0.6.0? which could potentially speed up the development of component libs.

@conficient
Copy link
Contributor

I'd just run into this bug trying to create bootstrap form components. It looks like anything in a RenderFragment doesn't seem to work, even with StateHasChanged being fired.

My test repo: https://github.com/conficient/BlazorBug01

There are two different problems here: first, a bind inside a parent RenderFragement only binds when it's accessed, and a bind-{propertyName} does not seem to fire onchange event at all.

Suggestions welcome.

@SteveSandersonMS
Copy link
Member

NOTE: When we fix this, we should also check the fix handles the scenarios raised at #1490, and if it doesn't we'll reopen #1490 as a separate issue.

@conficient
Copy link
Contributor

@danroth27 could I make a request to include this issue in 0.7.0, or failing that the next release? It appears this problem exists in server-side as well so would affect Razor Components going forward. Plus it will make creating re-usable components much easier (e.g. Bootstrap 4 layouts as in my repo are a classic example)

@danroth27
Copy link
Member

@conficient I'll optimistically put this in the milestone and we'll see if we can get to it.

@aspnet-hello aspnet-hello transferred this issue from dotnet/blazor Dec 17, 2018
@aspnet-hello aspnet-hello assigned rynowak and unassigned rynowak Dec 17, 2018
@aspnet-hello aspnet-hello added this to the 0.8.0 milestone Dec 17, 2018
@aspnet-hello aspnet-hello added 1 - Ready area-mvc Includes: MVC, Actions and Controllers, Localization, CORS, most templates bug This issue describes a behavior which is not expected - a bug. area-blazor Includes: Blazor, Razor Components labels Dec 17, 2018
@sbuchok
Copy link

sbuchok commented Jan 5, 2019

I created a fiddle that shows I believe the same issue:

https://blazorfiddle.com/s/861k07nk

If you change the value in the calendar, the value is updated in the component, but not in the parent.
If you then change the value in the textbox, the value for the calendar is changed to what was originally bound to it.

I hope this helps.

@danroth27
Copy link
Member

We expect to handle this as part of #6351

@mkArtakMSFT mkArtakMSFT removed area-mvc Includes: MVC, Actions and Controllers, Localization, CORS, most templates labels May 9, 2019
@ghost ghost locked as resolved and limited conversation to collaborators Dec 4, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-blazor Includes: Blazor, Razor Components bug This issue describes a behavior which is not expected - a bug.
Projects
None yet
Development

No branches or pull requests

10 participants