Skip to content
This repository was archived by the owner on Apr 12, 2024. It is now read-only.

feat($compile): add one-way binding to the isolate scope defintion #13928

Closed
wants to merge 6 commits into from
Closed
Show file tree
Hide file tree
Changes from 4 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
50 changes: 47 additions & 3 deletions src/ng/compile.js
Original file line number Diff line number Diff line change
Expand Up @@ -186,6 +186,28 @@
* you want to shallow watch for changes (i.e. $watchCollection instead of $watch) you can use
* `=*` or `=*attr` (`=*?` or `=*?attr` if the property is optional).
*
* * `<` or `<attr` - set up one-way (one-directional) binding between a local scope property and the
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the character should be > giving more feeling of forward movement

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As I wrote in #13854:

bindings: { propertyName: "<attrName" }
Which means: the value is copied from the attribute attrName to the property propertyName.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Trouble is that in my mind <attrName looks like a broken HTML element

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How about <=? It has the same connatation of moving from attribute to property (even more so) it has an element of = about it and looks nice propertyName: '<=attrName'

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Having this connotation is not as important as not having the opposite. As for multi-character sigils, I think they would look too scary in combination with ? and *:

bindings: { propertyName: "<=*?attrName" } // oh...

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm happy to go with the consensus and get this in :-)

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What was the reason - didn't make sense?
bindings: { propertyName: "-*?attrName" }

* parent scope property of name defined via the value of the `attr` attribute. If no `attr`
* name is specified then the attribute name is assumed to be the same as the local name.
* Given `<dir my-attr="parentModel">` and directive definition of
* `scope: { localModel:'<myAttr' }`, then isolate scope property `localModel` will reflect the
* value of `parentModel` on the parent scope. Any changes to `parentModel` will be reflected
* in `localModel`, but changes in `localModel` will not reflect in `parentModel`. There are however
* two caveats:
* 1. one-way binding does not copy the value from the parent to the isolate scope, it simply
* sets the same value. That means if your bound value is an object, changes to its properties
* in the isolate scope will be reflected in the parent scope.
* 2. one-way binding watches changes to the **identity** of the parent value. That is important should
* you one-way bind an object, and then replace that object in the isolated scope. If you now change
* a property of the object in your parent scope, the change will not be propagated to the isolated
* scope, because the identity of the object has not changed. Instead you must assign a new object.
*
* One-way binding is useful if you do not plan to propagate changes to your isolated scope bindings
* back to the parent. However, it does not make this completely impossible.
*
* Same as with bi-directional bindings, you can also use shallow watch for changes (i.e. $watchCollection instead of $watch):
* `<*` or `<*attr` (`<*?` or `<*?attr` if the property is optional).
*
* * `&` or `&attr` - provides a way to execute an expression in the context of the parent scope.
* If no `attr` name is specified then the attribute name is assumed to be the same as the
* local name. Given `<widget my-attr="count = count + value">` and widget definition of
Expand Down Expand Up @@ -826,7 +848,7 @@ function $CompileProvider($provide, $$sanitizeUriProvider) {
var EVENT_HANDLER_ATTR_REGEXP = /^(on[a-z]+|formaction)$/;

function parseIsolateBindings(scope, directiveName, isController) {
var LOCAL_REGEXP = /^\s*([@&]|=(\*?))(\??)\s*(\w*)\s*$/;
var LOCAL_REGEXP = /^\s*([@&]|[=<](\*?))(\??)\s*(\w*)\s*$/;

var bindings = {};

Expand Down Expand Up @@ -2962,7 +2984,7 @@ function $CompileProvider($provide, $$sanitizeUriProvider) {
optional = definition.optional,
mode = definition.mode, // @, =, or &
lastValue,
parentGet, parentSet, compare;
parentGet, parentSet, compare, removeWatch;

switch (mode) {

Expand Down Expand Up @@ -3023,7 +3045,6 @@ function $CompileProvider($provide, $$sanitizeUriProvider) {
return lastValue = parentValue;
};
parentValueWatch.$stateful = true;
var removeWatch;
if (definition.collection) {
removeWatch = scope.$watchCollection(attrs[attrName], parentValueWatch);
} else {
Expand All @@ -3032,6 +3053,29 @@ function $CompileProvider($provide, $$sanitizeUriProvider) {
removeWatchCollection.push(removeWatch);
break;

case '<':
if (!hasOwnProperty.call(attrs, attrName)) {
if (optional) break;
attrs[attrName] = void 0;
}
if (optional && !attrs[attrName]) break;

parentGet = $parse(attrs[attrName]);

destination[scopeName] = parentGet(scope);

if (definition.collection) {
removeWatch = scope.$watchCollection(parentGet, function parentCollectionValueWatchAction(newParentValue) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I do not understand what is the use case to use $watchCollection. We are not copying things from one side to the other, so I would just drop support for '<*', only support '<' (and its named and '?' versions) have the minimal viable feature and cut this release.

If, later on, people ask for this feature, then we can take the time to think if this is the way that best fits the needs.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree with @lgalfaso.

The only difference with <* is when the parent will be "synced" to the child and this only matters, if the child has set its property to a new instance. Since I can't think of any good usecase for setting the child to a new instance (knowing that at any point the parent could overwrite that value), I believe that <* will only cause confusion.

Keeping this feature as simple as possible (which means simpler semantics, less corner-cases, less unexpected behaviors) is pretty important imo.

As @lgalfaso said, we can always add stuff/features, but removing isn't that simple. Let's start simple.
(BTW, as @lgalfaso pointed out, there is no * in ng2, so I think it helps to be consistent.)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@lgalfaso Well spotted!

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I just found a use case for this today while using a 2-way binding (and wishing it was a 1-way binding)... when doing things like <element binding="array | orderBy:'foo'"> using a = binding causes an infinite digest while using =* works. But =* or <* really doesn't seem like a great solution for that, it would be great if there was some other way. Maybe things like filter and orderby should somehow return the same instance each time if it hasn't changed?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jbedard when you have <element binding="array | orderBy:'foo'"> we are not watching the result of the array after it went thru the filter, but before. Anyhow, if the behavior of filter and orderBy should change, then that should be another issue's discussion

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we are not watching the result of the array after it went thru the filter, but before

I don't think that's right. Just doing $rootScope.$watch("a | orderBy"); $rootScope.$apply("a = []") causes an infinite digest. I believe =* was created specifically to avoid this. However I think =* is a bad solution, it would be better if $parse/$watch handled this somehow...

destination[scopeName] = newParentValue;
});
} else {
removeWatch = scope.$watch(parentGet, function parentValueWatchAction(newParentValue) {
destination[scopeName] = newParentValue;
}, parentGet.literal);
}
removeWatchCollection.push(removeWatch);
break;

case '&':
// Don't assign Object.prototype method to scope
parentGet = attrs.hasOwnProperty(attrName) ? $parse(attrs[attrName]) : noop;
Expand Down
Loading