One way binding for objects in directive #12835

Closed
wants to merge 2 commits into
from

Projects

None yet

10 participants

@aoancea
aoancea commented Sep 13, 2015

While I was trying to build a modal with angular, i ended up with the problem of changing something in the modal pop-up and the changes were sync-ed with the original object.

Instead of doing all the work in each directive for copying the objects, i decided to extend the directive's operators to support one-way object binding.

I would like to also discuss about these changes if they are between the guidelines of angular js so we don`t pollute it with unwanted code.

This was tested on version 1.2.16(code is a little bit different there) and code was adjusted for this version.

If this gets approved by the community, I`ll upgrade my project to the latest version and have it tested fully.

aoancea added some commits Sep 13, 2015
@aoancea aoancea - new directive operator for one-way object binding in directive
fca036f
@aoancea aoancea - directive operator added in LOCAL_REGEXP
4c68c87
@googlebot

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project, in which case you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed, please reply here (e.g. I signed it!) and we'll verify. Thanks.


  • If you've already signed a CLA, it's possible we don't have your GitHub username or you're using a different email address. Check your existing CLA data and verify that your email is set on your git commits.
  • If you signed the CLA as a corporation, please let us know the company's name.
@googlebot googlebot added the cla: no label Sep 13, 2015
@aoancea
aoancea commented Sep 13, 2015

I signed it!

@googlebot

CLAs look good, thanks!

@googlebot googlebot added cla: yes and removed cla: no labels Sep 13, 2015
@gkalpak
Member
gkalpak commented Sep 14, 2015

Is this usecase common enough to warrant further complicating the DDO API ?

@aoancea
aoancea commented Sep 14, 2015

@gkalpak well this is one point I want to discuss, but I don`t see why we should replicate a certain piece of code all over when we can just let the framework do the job for us.

I'm happy to hear any other thoughts on the matter.

@gkalpak
Member
gkalpak commented Sep 14, 2015

I don`t see why we should replicate a certain piece of code all over when we can just let the framework do the job for us

The thing is that there are too many things that we can make the framework do for us.
Adding a new feature (usually) has good and bad side-effects:

  1. [GOOD] The users of the framework (the developers) have one more feature at their disposal.
    (Only some of them will probably use it though...)
  2. [BAD] The users of the framework (the developers) have to learn a more complicated or larger public API (regardless if they need a certain feature or not).
  3. [BAD] It increases the framework code-size. (Every end-user will have to download a bigger file, regardless if you (as a developer) make use of a feature or not.)
  4. [BAD] As the codebase increases, maintainance also gets harder meaning: more bugs sneak in, it is more difficult/timeconsuming to add new features/fix bugs.
    (This is usually of little to no relevance - only noticable for large or complicated features, touching big portions of the codebase .)

So, before adding a new feature, one has to weight (1) against (2) and (3).

Back to my original scepticism:

(Points (3) and (4) are not really relevant in this case, so fcusing on (1) and (2).)

I feel that the added feature is not common enough (i.e. not many developers need it/will use it), but it will add to the complexiy of the DDO (i.e. people will have to understand what # does in addition to =, @, &, ?), which is already too complicated and one of the main pain-points for new-comers.

But, let's see what other people think. They might find it a useful/common enough feature, to make it worth the complication of the DDO.


@aoancea, in any case, it is awesome that you took the time to put this together. 👍
Keep 'em coming 😃

@lgalfaso
Member

I agree with @gkalpak I do not find the use case common. An alternative would be to create a new directive that does this for you. Eg oneWayBind and would work <div some-directive one-way-bind="expression" one-way-bind-to="into"> and the implementation would watch oneWayBind on the child scope and copy the value to attr[oneWayBindTo] (ie [untested code] scope.$watch('oneWayBind', function(value) { attr[oneWayBindTo] = angular.copy(value); });

@Narretz Narretz added this to the Ice Box milestone Sep 14, 2015
@aoancea
aoancea commented Sep 15, 2015

@gkalpak very nice justification. I totally agree with all of the points you mentioned in there, that's why I even wanted to discuss about it.

Probably I should have raised an issue on the matter, but having the code in front of us, as we are all developers, I thought that it would be easier to understand the problem and talk on the matter.

Also thanks for appreciating my contribution, and if you have any points of attention please share as I`m new to this contribution stuff.

@lgalfaso I thought about doing that at the start, but adding a new operator seemed more interesting. Ill definitely have a look over what youre suggesting and thanks.

@petebacondarwin
Member

I think there may be some benefit in adding this in the case where we want to move more toward a single direction of data flow - properties down and events up. This would be beneficial to the ng-forward project.

@timkindberg
Contributor

Seconded what @petebacondarwin said. We currently add one-way binding via a kind of hack. It would be nice to just use an official feature to get this functionality.

@MikeRyan52
Member

There is a lot of value in a one way operator for components that have similar behavior to Angular 2 components (date flows down, events bubble up). If you wanted to build an entire Angular 1 app this way, it wouldn't be feasible to use a oneWayBind directive for all bindings. Would really like to see this feature as a scope binding operator land in core.

@aoancea: I can take on getting this finished up if need be!

@aoancea
aoancea commented Nov 7, 2015

@MikeRyan52 I already solved the problem with what @lgalfaso suggested.

Here is the link to it: http://computerscienceunveiled.blogspot.ro/2015/09/angular-modal-directive-with-one-way.html

What I'd still do is replace the hard-coded operators with some configurable ones. Those 3 that are now should be included by default, but you should have the possibility of adding your own. In this way we could have extensibility at a very low cost. I'm thinking of a way in which interceptors are added now.

@robstove

My experience is that the need for one-way binding is more common than two-way especially for directives. We try to keep our watches to a minimum and for most scenarios we don't want a directive changing the model it was given. Our directives offer a means of passing objects to them via binding and that can only be done with two-way or '&' binding. Two-way binding is a problem because you end up with two watches, one of which isn't needed. '&' binding is a problem because of the semantics involved.

I'll add another example that may help with motivation. We have built various directives that take an array of objects given to it by the consumer. Two-way binding doesn't make sense here because the directive will never alter the array. So we're left with using '&' binding which then puts a rather strange requirement on the consumer to give us an expression to deliver the array. Having the option to specify one-way binding in this case would solve this and many other scenarios we have as most data in our application flows into the directives and only flows out through events.

Both one-way and two-way binding would look the same from the consumer standpoint.
<data-list items-source="myArrayOfObjects"

vs. '&' binding which has poor semantics.
<data-list get-items-source="getMyArrayOfObjects()"

$0.02

@gkalpak
Member
gkalpak commented Jan 31, 2016

I just wanted to mention that one-way-bindings, won't prevent a directive from altering the data (unless you pass primitives or immutable/frozen objects).

@petebacondarwin
Member

I just wanted to mention that one-way-bindings, won't prevent a directive from altering the data (unless you pass primitives or immutable/frozen objects).

@gkalpak that depends upon how the feature is implemented, no? If we always took a copy of objects in a one-way binding watcher then we could modify the inner version to our heart's content without affecting anything in the outer scope, which I believe is the result that we would want from such a feature.

I am still keen to see this happen but I appreciate that it would need to be thought through carefully to prevent unwanted loss of performance, etc.

@robstove

Good point @gkalpak. I suppose whether or not the data is altered by a directive is more of an implementation detail although by adding support for one-way binding allows the responsibility to be communicated more effectively. This could also create an opportunity for the directive to work on a copy of the provided binding data. A copy would allow data to be in the states of original and working with the flexibility for the consumer to commit the working copy to the original based on some event or state.

@gkalpak
Member
gkalpak commented Feb 1, 2016

If we always took a copy of objects in a one-way binding watcher then we could modify the inner version to our heart's content without affecting anything in the outer scope

That wouldn't be so straight-forward, I think. For copying to make sense as a way to prevent modification, it needs to be a deep copy and deep copying arbitrary objects isn't a solved problem :)

TBH, I don't think we can get away with copying the object, unless we set some hard rules about the kind of properties that are allowed on such an object (and its "sub-objects"), which would make the feature kind of restrictive.

@petebacondarwin
Member

What is the Angular 2 strategy in this regard?

@gkalpak
Member
gkalpak commented Feb 1, 2016

They pass the object as is, so if you change a property then the change is reflected back to the parent.
Demo plnkr

@petebacondarwin petebacondarwin modified the milestone: 1.5.0, Ice Box Feb 1, 2016
@petebacondarwin
Member

So the key point is that we don't make a copy but we do only watch the outer value for (identity) changes and don't overwrite the inner value unless the outer value (identity) actually changes.

@gkalpak
Member
gkalpak commented Feb 1, 2016

Sounds reasonable.

I just think some people will confuse one-way binding with unidirectional data flow, so we should make it clear (e.g. in docs) that it's not the same thing 😃

@thorn0
Contributor
thorn0 commented Feb 1, 2016

BTW, @robstove, looks like you're missing the fact that the attributes for &-bindings don't have to look like you wrote:

<data-list get-items-source="getMyArrayOfObjects()">

The term 'expression' doesn't mean it has to be a function call. This works fine too:

<data-list get-items-source="myArrayOfObjects">

However, these bindings don't setup watches at all whereas it's usually desirable that a component react to changes in its inputs.

@robstove
robstove commented Feb 1, 2016

@thorn0 Yes of course. Its a problem of semantics. If I use an expression I would expect the semantics of the expression to look like function rather than a property because we are evaluating something. It's not that we can't do these things it's a matter of doing things in a way that make them easy to communicate.

@thorn0
Contributor
thorn0 commented Feb 1, 2016

@robstove I read your "from the consumer standpoint" like you meant the consumer of the directive, the one who uses it and never looks into its source code. For such a person, = and & can look indistinguishable. Outwardly, not inside the directive's code of course.

@thorn0
Contributor
thorn0 commented Feb 1, 2016

@petebacondarwin

So the key point is that we don't make a copy but we do only watch the outer value for (identity) changes and don't overwrite the inner value unless the outer value (identity) actually changes.

Sounds right. It should work totally like =, but without modifying the parent. From my experience, most of the time modifying the parent isn't needed and can help bugs propagate.

@thorn0
Contributor
thorn0 commented Feb 1, 2016

@gkalpak

some people will confuse one-way binding with unidirectional data flow, so we should make it clear (e.g. in docs) that it's not the same thing

They're not the same, but the former is needed to implement the latter.

@gkalpak
Member
gkalpak commented Feb 1, 2016

They're not the same, but the former is needed to implement the latter.

I'm not sure it is. Unidirectional data flow is more of an app implementation decision that a framework feature. AFAICT, one-way binding isn't necessary for unidirectional data flow.

@thorn0
Contributor
thorn0 commented Feb 1, 2016

I'm not sure it is. Unidirectional data flow is more of an app implementation decision that a framework feature.

Perhaps I should have put it like this: it's convenient to use the former to implement the latter. Implementation decisions are uh... implemented using framework features, aren't they? And a framework can be more friendly to some decisions and stand in the way, to a greater or lesser extent, of implementing others.

one-way binding isn't necessary for unidirectional data flow.

Yep. Good old two-way bindings can be used too. They just require more discipline.

@timkindberg
Contributor

I'm not sure it is.

Not necessary, but somewhat dumb without it. I've currently got a unidirectional application and if it wasn't for disciplined immutability on my part I'd be two-way binding to all kinds of things I don't want to change. It was overly tricky to set up a unidirectional data flow with two way binding via '='.

@gkalpak
Member
gkalpak commented Feb 2, 2016

I've currently got a unidirectional application and if it wasn't for disciplined immutability on my part I'd be two-way binding to all kinds of things I don't want to change.

I dont really see how this will change with one-way binding.

Let's assume one wants to avoid having child components mutating data that is passed to them from a parent component. Let's also assume that parent.parentData is passed from the parent component and bound to the child component as child.childData.
With the current two-way binding, parentData can me modified by the child component in two ways (maybe that's why the call it two-way bindings 😛):

  1. Change a property of child.childData (which is just a reference to parent.parentData), e.g.: child.childData.someProp = 'value from child'. Any change made to the object reference acquired from the parent component, will reflect back to the parent, since it's the same object.
  2. Set a new value for child.childData, e.g.: child.childData = {/* some new object */}. Due to two-way binding, parent.parentData will also be set that new object (i.e.: parent.parentData = child.childData).

Now, with one-way bindings, only the second way of modifying parent data from the child component will be prevented. And it will be prevented in a (nasty imo) "silently fail" way: The changes will be visible in the child component only, until (based on the implementation) some event makes the parent data overwrite those changes in the child component.

IMO, one-way binding requires even more discipline to implement unidirectional data flow. And there is also the caveat, that people might misinterpret one-way binding and get a false sense of "unidirectional-data-flow-ity".

I'm still in favor of adding one-way binding, because it can reduce unnecessary watchers in certain situations where it's more appropriate than two-way bindings, but there are several things to take into careful consideration and properly communicate to devs.

@petebacondarwin petebacondarwin added a commit that closed this pull request Feb 3, 2016
@Narretz @petebacondarwin Narretz + petebacondarwin feat($compile): add one-way binding to the isolate scope definition
This change allows the developer to bind an isolate scope / controller property
to an expression, using a `<` binding, in such a way that if the value of the
expression changes, the scope/controller property is updated but not the
converse.

This makes it easier to build AngularJS applications that have unidirectional
data flow.

The binding is implemented simply as a single watch, which can also provide
performance benefits over two way bindings.

Closes #13928
Closes #13854
Closes #12835
Closes #13900
92374f9
@petebacondarwin petebacondarwin added a commit that referenced this pull request Feb 3, 2016
@Narretz @petebacondarwin Narretz + petebacondarwin feat($compile): add one-way binding to the isolate scope definition
This change allows the developer to bind an isolate scope / controller property
to an expression, using a `<` binding, in such a way that if the value of the
expression changes, the scope/controller property is updated but not the
converse.

The binding is implemented as a single simple watch, which can also provide
performance benefits over two way bindings.

Closes #13928
Closes #13854
Closes #12835
Closes #13900
4ac23c0
@LeeAdcock LeeAdcock added a commit to LeeAdcock/angular.js that referenced this pull request Feb 17, 2016
@Narretz @LeeAdcock Narretz + LeeAdcock feat($compile): add one-way binding to the isolate scope definition
This change allows the developer to bind an isolate scope / controller property
to an expression, using a `<` binding, in such a way that if the value of the
expression changes, the scope/controller property is updated but not the
converse.

The binding is implemented as a single simple watch, which can also provide
performance benefits over two way bindings.

Closes #13928
Closes #13854
Closes #12835
Closes #13900
8a7676e
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment