New bindings updating the scope when created #2020

Closed
dylanrtt opened this Issue Oct 23, 2015 · 13 comments

Comments

Projects
None yet
4 participants
@dylanrtt
Contributor

dylanrtt commented Oct 23, 2015

With the recent changes to how bindings work, there has been a new addition which allows an element (e.g., form control, component) to update the scope if the element has a value and the scope does not (with 2-way bindings). This happens on bindings.js#L539 when the binding is created.

Perhaps someone can explain to me why this feature needs to exist because I ran into some issues with it today. For a while now, it has seemed like the scope (or parent scope) is the absolute source of truth. However, with these new rules, it is less clear. Now whoever has a value first wins.

In #1834, I explained a bit about why I think this is a bad idea. It totally violates top->down data flow and opens up a whole new can of potential debugging nightmares. In the case of select elements, I explained how the form control can be changed to accurately reflect the scope. For other form elements, I don't see how this feature would be seen as necessary.

Especially in the case of components, I question whether a child should be updating the parent scope with two-way="{bindings}" if the parent doesn't have a value. Framework users could run into upgrade issues with this behavior changing in 2.3.0. Perhaps they didn't even want 2-way binding in the first place but there was no other way to pass down an object or something. What if the parent is setting a value async and the child has a default... that could have unexpected consequences.

I'm not sure how much thought and discussion was put into making this change but I think this feature should still be on the table.

@matthewp

This comment has been minimized.

Show comment
Hide comment
@matthewp

matthewp Oct 23, 2015

Contributor

I'm not sure how much thought and discussion was put into making this change but I think this feature should still be on the table.

A whole lot of both thought and discussion was put into it. And it is on the table for 3.0, not for 2.3, but 3.0 is next. That's why we released this now, because we know it's a big change and want to get feedback on how it works in practice and make adjustment if needed in 3.0.

But to answer some of your question, correct me if I'm wrong but it sounds like you want one-way binding behavior, you know that you have that now, right? Previously it was always two-way. You can do {child}="parent" to get 1 way binding which is what it sounds like you want.

Contributor

matthewp commented Oct 23, 2015

I'm not sure how much thought and discussion was put into making this change but I think this feature should still be on the table.

A whole lot of both thought and discussion was put into it. And it is on the table for 3.0, not for 2.3, but 3.0 is next. That's why we released this now, because we know it's a big change and want to get feedback on how it works in practice and make adjustment if needed in 3.0.

But to answer some of your question, correct me if I'm wrong but it sounds like you want one-way binding behavior, you know that you have that now, right? Previously it was always two-way. You can do {child}="parent" to get 1 way binding which is what it sounds like you want.

@justinbmeyer

This comment has been minimized.

Show comment
Hide comment
@justinbmeyer

justinbmeyer Oct 23, 2015

Contributor

@dylanrtt a decent amount of thought has been put into this. Two way binding values to the references scope is an important and commonly used feature in many of the apps that have begun to use it.

<date-picker {(time)}="*date"/>
<date-slider {(time)}="*date"/>

We need *date set to the child time if either of these controls has a value.

With giving people the ability to control the directions of bindings, most scenarios where this is a problem (child has a value that you actually want to set to undefined) are very minimal.

You are right that this could cause some breaking changes with upgrading. And while the behavior was never documented (it is now), if this problem arises, I can make the old binding syntax follow the old behavior.

I agree that it violates top-down data flow, but I don't think it opens a bunch of debugging problems. The initial value might be surprising (I thought the old behavior was surprising) , but binding from that point forward will behave the same was as previously. I think debugging "bindings" is more complex then initial value problems.

This behavior is settled for 2.3. But 3.0 is next. For 3.0, we could perhaps control the rules with changes like:

  • {()} -> parent wins
  • ({}) -> child wins
Contributor

justinbmeyer commented Oct 23, 2015

@dylanrtt a decent amount of thought has been put into this. Two way binding values to the references scope is an important and commonly used feature in many of the apps that have begun to use it.

<date-picker {(time)}="*date"/>
<date-slider {(time)}="*date"/>

We need *date set to the child time if either of these controls has a value.

With giving people the ability to control the directions of bindings, most scenarios where this is a problem (child has a value that you actually want to set to undefined) are very minimal.

You are right that this could cause some breaking changes with upgrading. And while the behavior was never documented (it is now), if this problem arises, I can make the old binding syntax follow the old behavior.

I agree that it violates top-down data flow, but I don't think it opens a bunch of debugging problems. The initial value might be surprising (I thought the old behavior was surprising) , but binding from that point forward will behave the same was as previously. I think debugging "bindings" is more complex then initial value problems.

This behavior is settled for 2.3. But 3.0 is next. For 3.0, we could perhaps control the rules with changes like:

  • {()} -> parent wins
  • ({}) -> child wins
@dylanrtt

This comment has been minimized.

Show comment
Hide comment
@dylanrtt

dylanrtt Oct 23, 2015

Contributor

@justinbmeyer I think this behavior makes perfect sense for the references scope, but updating the parent viewmodel is a different subject and increases complexity of an app, which is perhaps part of why you were originally against {^child}="parent" bindings.

Sorry if I'm not very forgiving of breaking changes, but I have used CanJS for a little over a year now and I can't even count the number of times I have had to deal with them, let alone the dozens of hours I have spent debugging them, all on 2.X.

Many times the developer whose code I was updating just "did it wrong" due to lack of proper documentation or functionality needed. Sometimes you guys just decided to change something you didn't like, which was possible because you conveniently never documented it. Other times, issues have arisen by doing it exactly the way you demonstrate, such as nesting #eq statements which turns out to be problematic due to the fundamental flaw of child bindings receiving updates first.

Many of the times I have upgraded were for mandatory bug fixes or for features I felt handicapped without. Nearly every time, I had to deal with breaking changes or I would discover new bugs, requiring me to yet again upgrade and have to deal with more of the same, trading one set of bugs for another. I don't recall ever feeling a sense of stability with this framework, and as much as I really hate saying this publicly (edit it out if you want), I'm not sure I can honestly recommend to anyone that they start a new project using CanJS.

Don't get me wrong, though. I do like many of the framework's features and the general direction things are headed. I appreciate all the hard work you guys do to improve the framework and add features like references scope, passing functions, and better binding syntax, all of which I am using already and love.

Contributor

dylanrtt commented Oct 23, 2015

@justinbmeyer I think this behavior makes perfect sense for the references scope, but updating the parent viewmodel is a different subject and increases complexity of an app, which is perhaps part of why you were originally against {^child}="parent" bindings.

Sorry if I'm not very forgiving of breaking changes, but I have used CanJS for a little over a year now and I can't even count the number of times I have had to deal with them, let alone the dozens of hours I have spent debugging them, all on 2.X.

Many times the developer whose code I was updating just "did it wrong" due to lack of proper documentation or functionality needed. Sometimes you guys just decided to change something you didn't like, which was possible because you conveniently never documented it. Other times, issues have arisen by doing it exactly the way you demonstrate, such as nesting #eq statements which turns out to be problematic due to the fundamental flaw of child bindings receiving updates first.

Many of the times I have upgraded were for mandatory bug fixes or for features I felt handicapped without. Nearly every time, I had to deal with breaking changes or I would discover new bugs, requiring me to yet again upgrade and have to deal with more of the same, trading one set of bugs for another. I don't recall ever feeling a sense of stability with this framework, and as much as I really hate saying this publicly (edit it out if you want), I'm not sure I can honestly recommend to anyone that they start a new project using CanJS.

Don't get me wrong, though. I do like many of the framework's features and the general direction things are headed. I appreciate all the hard work you guys do to improve the framework and add features like references scope, passing functions, and better binding syntax, all of which I am using already and love.

@justinbmeyer

This comment has been minimized.

Show comment
Hide comment
@justinbmeyer

justinbmeyer Oct 23, 2015

Contributor

@dylanrtt I'm sorry about those breaking changes. And, I appreciate you voicing your concern.

I don't think there's any cases since 2.0 where we have broken documented features.

But, I admit that we've played fast and lose with undocumented features. The lack of documentation isn't a connivence. If things are not specified, it's often for a reason. We are unsure of the behavior.

Most of the times when we change things, it's for good reason.

I'm guessing, for instance, that when you upgraded to 2.2, some computed value behavior was different. Upgrading broke several of our apps too. This change fixed a bug with computes getting re-evaluted too many times. The change didn't break any documented APIs so it wasn't a 3.0.

The behavior of the new component bindings during initialization is actually now specified because we finally came to some agreement on it.

Fixing the #eq problem is high priority once DoneJS it out.

If this is the tipping point that makes you not recommend CanJS, I regret that. However, I will leave this issue open and get out a 2.3.1 with the old binding behavior restored.

If there are other things that you find breaking, please let us know. By us labeling it 2.3 it means it's API complete, not perfect. There still might be regressions not found by our test suite. We're happy to fix them.

Contributor

justinbmeyer commented Oct 23, 2015

@dylanrtt I'm sorry about those breaking changes. And, I appreciate you voicing your concern.

I don't think there's any cases since 2.0 where we have broken documented features.

But, I admit that we've played fast and lose with undocumented features. The lack of documentation isn't a connivence. If things are not specified, it's often for a reason. We are unsure of the behavior.

Most of the times when we change things, it's for good reason.

I'm guessing, for instance, that when you upgraded to 2.2, some computed value behavior was different. Upgrading broke several of our apps too. This change fixed a bug with computes getting re-evaluted too many times. The change didn't break any documented APIs so it wasn't a 3.0.

The behavior of the new component bindings during initialization is actually now specified because we finally came to some agreement on it.

Fixing the #eq problem is high priority once DoneJS it out.

If this is the tipping point that makes you not recommend CanJS, I regret that. However, I will leave this issue open and get out a 2.3.1 with the old binding behavior restored.

If there are other things that you find breaking, please let us know. By us labeling it 2.3 it means it's API complete, not perfect. There still might be regressions not found by our test suite. We're happy to fix them.

@justinbmeyer

This comment has been minimized.

Show comment
Hide comment
@justinbmeyer

justinbmeyer Oct 23, 2015

Contributor

Also, what you are experiencing is a bit by design (though I want to be getting ever more stable). Instead of releasing 3.0 with HUGE changes from 2.0 (like angular 1.0 to 2.0), we make a lot of stepwise changes. It's annoying more often, but when stretched out, we think it's much more likely those updates happen.

3.0 will be largely backwards compatible to 2.3. Ideally, they will be the same except for some defaults will be switched (ex: mustache -> stache).

And hopefully, these regressions, as our test suite keeps growing and growing, will be fewer and further between.

Contributor

justinbmeyer commented Oct 23, 2015

Also, what you are experiencing is a bit by design (though I want to be getting ever more stable). Instead of releasing 3.0 with HUGE changes from 2.0 (like angular 1.0 to 2.0), we make a lot of stepwise changes. It's annoying more often, but when stretched out, we think it's much more likely those updates happen.

3.0 will be largely backwards compatible to 2.3. Ideally, they will be the same except for some defaults will be switched (ex: mustache -> stache).

And hopefully, these regressions, as our test suite keeps growing and growing, will be fewer and further between.

@justinbmeyer

This comment has been minimized.

Show comment
Hide comment
@justinbmeyer

justinbmeyer Oct 24, 2015

Contributor

One more thing ... hopefully final ...

I think what could help a lot is breaking up CanJS into individual projects. If you could have different versions of can.Component and can.stache, it's probable you could upgrade one for bug fixes without the other.

This way bug fixes wouldn't have to be bundled with feature or breaking changes.

I think it's possible to still have a core "can" that includes certain versions of things ... similar to what we are going to do with DoneJS. But for advanced people ... this means we could probably get new releases and fixes out much much faster.

Contributor

justinbmeyer commented Oct 24, 2015

One more thing ... hopefully final ...

I think what could help a lot is breaking up CanJS into individual projects. If you could have different versions of can.Component and can.stache, it's probable you could upgrade one for bug fixes without the other.

This way bug fixes wouldn't have to be bundled with feature or breaking changes.

I think it's possible to still have a core "can" that includes certain versions of things ... similar to what we are going to do with DoneJS. But for advanced people ... this means we could probably get new releases and fixes out much much faster.

@justinbmeyer

This comment has been minimized.

Show comment
Hide comment
@justinbmeyer

justinbmeyer Oct 29, 2015

Contributor

@Alfredo-Delgado Here's where the child->parent updating is taking place: https://github.com/bitovi/canjs/blob/master/component/component.js#L236

The fix would be to track on bindingsData what type of binding it is .. a "new" syntax binding or an "old" syntax binding. If it's an "old" syntax binding, don't update the parent.

Contributor

justinbmeyer commented Oct 29, 2015

@Alfredo-Delgado Here's where the child->parent updating is taking place: https://github.com/bitovi/canjs/blob/master/component/component.js#L236

The fix would be to track on bindingsData what type of binding it is .. a "new" syntax binding or an "old" syntax binding. If it's an "old" syntax binding, don't update the parent.

Alfredo-Delgado added a commit that referenced this issue Oct 30, 2015

Previous behavior restored for "old" two-way binding syntax -- `foo="…
…{bar}"`. A child property shall not initialize an undefined parent property. Resolves #2020

Alfredo-Delgado added a commit that referenced this issue Oct 30, 2015

Previous behavior restored for "old" two-way binding syntax -- `foo="…
…{bar}"`. A child property shall not initialize an undefined parent property. Resolves #2020
@dylanrtt

This comment has been minimized.

Show comment
Hide comment
@dylanrtt

dylanrtt Nov 12, 2015

Contributor

Dang, I was hoping this was already fixed in 2.3.1 but it looks like it's going to be in 2.3.2.

I ran into an issue today because the old syntaxes were using the new binding behavior...

<my-comp a="{foo}" b="{foo.bar}"></my-comp>

my-comp.b was defined as defaulting to the value of my-comp.a, and foo.bar was undefined. This resulted in the initial value of my-comp.b (equal to my-comp.a and foo) getting assigned to foo.bar, meaning foo === foo.bar, which was very unexpected. I only found it because foo.save() failed due to a stack overflow from circular references.

http://jsbin.com/wewobizonu/edit?html,js,console,output

I think this is a good example of why these binding rules can be bad, especially for backwards compatibility. The obvious solution in this instance is to simply change it to a one-way binding (if applicable) or make sure foo.bar is defined initially, but no change should be necessary if you do not change the binding syntax so I am glad you guys decided to revert the binding rules (edit: for the old syntax). I think it was the right choice.

Contributor

dylanrtt commented Nov 12, 2015

Dang, I was hoping this was already fixed in 2.3.1 but it looks like it's going to be in 2.3.2.

I ran into an issue today because the old syntaxes were using the new binding behavior...

<my-comp a="{foo}" b="{foo.bar}"></my-comp>

my-comp.b was defined as defaulting to the value of my-comp.a, and foo.bar was undefined. This resulted in the initial value of my-comp.b (equal to my-comp.a and foo) getting assigned to foo.bar, meaning foo === foo.bar, which was very unexpected. I only found it because foo.save() failed due to a stack overflow from circular references.

http://jsbin.com/wewobizonu/edit?html,js,console,output

I think this is a good example of why these binding rules can be bad, especially for backwards compatibility. The obvious solution in this instance is to simply change it to a one-way binding (if applicable) or make sure foo.bar is defined initially, but no change should be necessary if you do not change the binding syntax so I am glad you guys decided to revert the binding rules (edit: for the old syntax). I think it was the right choice.

@justinbmeyer

This comment has been minimized.

Show comment
Hide comment
@justinbmeyer

justinbmeyer Nov 12, 2015

Contributor

To be clear, we are not reverting the binding rules for the new bindings.

Contributor

justinbmeyer commented Nov 12, 2015

To be clear, we are not reverting the binding rules for the new bindings.

@justinbmeyer

This comment has been minimized.

Show comment
Hide comment
@justinbmeyer

justinbmeyer Nov 12, 2015

Contributor

I don't think foo === foo.bar is unexpected as long as you know that undefined parent values will be set to their child value. It should be clear that foo.bar is undefined, so it will take on the value of b, which will be a.

Contributor

justinbmeyer commented Nov 12, 2015

I don't think foo === foo.bar is unexpected as long as you know that undefined parent values will be set to their child value. It should be clear that foo.bar is undefined, so it will take on the value of b, which will be a.

@dylanrtt

This comment has been minimized.

Show comment
Hide comment
@dylanrtt

dylanrtt Nov 12, 2015

Contributor

I don't think foo === foo.bar is unexpected as long as you know that undefined parent values will be set to their child value.

Except my code pre-existed that rule and running npm install can@2.3.0 did not impart that knowledge 😛 (might be a bug with npm). To be clear, I'm saying it's good that the old syntax will continue to work the old way to prevent unexpected things like this for people upgrading.

Contributor

dylanrtt commented Nov 12, 2015

I don't think foo === foo.bar is unexpected as long as you know that undefined parent values will be set to their child value.

Except my code pre-existed that rule and running npm install can@2.3.0 did not impart that knowledge 😛 (might be a bug with npm). To be clear, I'm saying it's good that the old syntax will continue to work the old way to prevent unexpected things like this for people upgrading.

@justinbmeyer

This comment has been minimized.

Show comment
Hide comment
@justinbmeyer

justinbmeyer Nov 12, 2015

Contributor

2.3.2 should be out tomorrow. There's one issue to go. I'm working on it now.

If you'd like to be part of these decisions, send me an email: justin@bitovi.com. We're looking for contributors and it's pretty easy to be part of the team: http://donejs.com/About.html#section=section_Coreteam

Contributor

justinbmeyer commented Nov 12, 2015

2.3.2 should be out tomorrow. There's one issue to go. I'm working on it now.

If you'd like to be part of these decisions, send me an email: justin@bitovi.com. We're looking for contributors and it's pretty easy to be part of the team: http://donejs.com/About.html#section=section_Coreteam

@justinbmeyer

This comment has been minimized.

Show comment
Hide comment
@justinbmeyer

justinbmeyer Nov 12, 2015

Contributor

Except my code pre-existed that rule and running npm install can@2.3.0 did not impart that knowledge 😛 (might be a bug with npm).

Yeah, regressions will occur, especially if functionality is untested and not documented, but we will fix it in patch releases.

Contributor

justinbmeyer commented Nov 12, 2015

Except my code pre-existed that rule and running npm install can@2.3.0 did not impart that knowledge 😛 (might be a bug with npm).

Yeah, regressions will occur, especially if functionality is untested and not documented, but we will fix it in patch releases.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment