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

Narretz · 2016-02-02T13:02:08Z

No description provided.

gkalpak · 2016-02-02T13:17:45Z

src/ng/compile.js

+            }
+
+            if (definition.collection) {
+              removeWatch = scope.$watchCollection($parse(attrs[attrName]), onParentValueChange);


Couldn't parentGet be used here (to avoid double parsing) ?

Do we even need to parse this? We are just passing the string to watch, which parses this anyway. Yeah, I think I was confused about what's going oon in the two-way binding

I think we should parse it to init the child value. However, you can do this

scope.$watchCollection(parentGet, ...)

to not parse twice.

gkalpak · 2016-02-02T13:38:52Z

test/ng/compileSpec.js

@@ -4219,6 +4227,299 @@ describe('$compile', function() {
    });


+    describe('one-way binding', function() {
+      it('should update isolate when the identity of origin changes', inject(function() {


Unecessary inject() (here and below) ?

They are necessary. (many other tests have them). I think it's because the $rootScope etc. "globals" are set up in a module'S return fn.

campersau · 2016-02-02T14:31:44Z

src/ng/compile.js

+                destination[scopeName] = newParentValue;
+              });
+            } else {
+              removeWatch = scope.$watch(attrs[attrName], function onParentCollectionValueChange(newParentValue) {


Are the function names for the watch callbacks swapped?
Should't it be onParentValueChange here and onParentCollectionValueChange above?

gkalpak · 2016-02-02T17:42:08Z

test/ng/compileSpec.js

+            owRefAlias: '< owRef',
+            owOptref: '<?',
+            owOptrefAlias: '<? owOptref',
+            owOptreference: '<?',


OOC, what is the purpose of owOptref + owOptreference ?

Came from a c&p'd test, but owOptreference doesn't look like it's needed.

thorn0 · 2016-02-02T19:17:33Z

Currently, if an object literal or an array literal is passed as the expression for a two-way (=) binding, the set up watch is deep (uses angular.equals). The implementation in this PR doesn't do this with the new one-way bindings. IMO it'd more consistent if one-way bindings behaved exactly like two-way bindings, just without probability for the parent to get modified by them (with the known caveats).

gkalpak · 2016-02-02T19:27:19Z

test/ng/compileSpec.js

+
+          $rootScope.name = 'c';
+          $rootScope.$apply();
+          expect(componentScope.owRef).toEqual({name: 'c'});


I expected it to remain {name: 'b'}. Interesting...

Aha, parsed expression with inputs and $$watchDelegates. Nice 😃

That's why it works? I'm actually confused by this right now.

Yes. Parsing {name: name} the resulting parsed expression has inputs, so it has a $$watchDelegate that will re-evaluate the expression whenever the inputs change value (in this case, inputs is the name property on $rootScope).
So, whenever $rootScope.name changes, the expression will be evaluated and will return a new object, thus the change will be propagated to the child.

So this works, but when you are going deeper, you need objectEquality watch - I've added that right now.

gkalpak · 2016-02-02T19:28:39Z

test/ng/compileSpec.js

+        describe('optional one-way binding', function() {
+          it('should update local when origin changes', inject(function() {
+            compile('<div><span my-component ow-optref="name">');
+            expect(componentScope.owOptRef).toBe(undefined);


owOptRef --> owOptref

Narretz · 2016-02-02T20:27:17Z

Hm, something strange is going on. The watchAction functions fire twice every time there's an apply. I don't think this should happen. Maybe that's also why the literals work even though they shouldn't.

Ah, it's too late. This comes from the alias that is on ow-ref. All good.

gkalpak · 2016-02-02T20:29:31Z

A couple of minor comments, but LGTM otherwise, expect for the fact that we should be more detailed on what is updated when and what is the difference between * and no *.

lgalfaso · 2016-02-02T21:07:50Z

src/ng/compile.js

+            destination[scopeName] = parentGet(scope);
+
+            if (definition.collection) {
+              removeWatch = scope.$watchCollection(parentGet, function parentCollectionValueWatchAction(newParentValue) {


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.

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.)}

@lgalfaso Well spotted!

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?

@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

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...

Narretz · 2016-02-03T10:57:22Z

I've

removed the collectionWatch functionality
updated the description to incorporate changes made by @thorn0
added watching literals by object equality

gkalpak · 2016-02-03T12:42:54Z

src/ng/compile.js

+ *   parent scope. If no `attr` name is specified then the attribute name is assumed to be the same as the
+ *   local name. You can also make the binding optional by adding `?`: `<?` or `<?attr`.
+ *
+ *   For example, given `<dir my-attr="parentModel">` and directive definition of


dir --> my-component (to be consistent with #13921)

gkalpak · 2016-02-03T12:50:31Z

LGTM (expect for some minor docs typos/fixes).
After this gets merged, it might be a goood idea to add an example in the Directives guide.

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

Narretz added type: feature component: $compile frequency: high needs: review labels Feb 2, 2016

Narretz added this to the 1.5.0 milestone Feb 2, 2016

googlebot added the cla: yes label Feb 2, 2016

Narretz force-pushed the feat-compile-oneway branch from 6987dd8 to bd1f619 Compare February 2, 2016 13:06

gkalpak reviewed Feb 2, 2016
View reviewed changes

Narretz force-pushed the feat-compile-oneway branch from bd1f619 to ca8cc76 Compare February 2, 2016 13:34

gkalpak reviewed Feb 2, 2016
View reviewed changes

Narretz force-pushed the feat-compile-oneway branch from ca8cc76 to 218b338 Compare February 2, 2016 14:14

campersau reviewed Feb 2, 2016
View reviewed changes

Narretz force-pushed the feat-compile-oneway branch from 218b338 to 93a857b Compare February 2, 2016 15:45

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

31d31e4

Narretz force-pushed the feat-compile-oneway branch from 93a857b to 31d31e4 Compare February 2, 2016 15:51

gkalpak reviewed Feb 2, 2016
View reviewed changes

fixes

b7389f5

update test expectation

35f5b5b

Narretz force-pushed the feat-compile-oneway branch from c0b6bdc to 35f5b5b Compare February 2, 2016 19:22

gkalpak reviewed Feb 2, 2016
View reviewed changes

deep-watch object literals, fix typos

e141438

lgalfaso reviewed Feb 2, 2016
View reviewed changes

remove shallow watch, update docs

368ce25

gkalpak reviewed Feb 3, 2016
View reviewed changes

fix typos, wordings

e5db80d

petebacondarwin self-assigned this Feb 3, 2016

petebacondarwin closed this in 92374f9 Feb 3, 2016

thorn0 mentioned this pull request Feb 14, 2016

One-way bindings + shallow watching #14039

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

Narretz commented Feb 2, 2016

gkalpak Feb 2, 2016

Narretz Feb 2, 2016

thorn0 Feb 2, 2016

gkalpak Feb 2, 2016

Narretz Feb 2, 2016

campersau Feb 2, 2016

gkalpak Feb 2, 2016

Narretz Feb 2, 2016

thorn0 commented Feb 2, 2016

gkalpak Feb 2, 2016

gkalpak Feb 2, 2016

Narretz Feb 2, 2016

gkalpak Feb 2, 2016

Narretz Feb 2, 2016

gkalpak Feb 2, 2016

Narretz commented Feb 2, 2016

gkalpak commented Feb 2, 2016

lgalfaso Feb 2, 2016

gkalpak Feb 2, 2016

thorn0 Feb 3, 2016

jbedard Feb 4, 2016

lgalfaso Feb 4, 2016

jbedard Feb 4, 2016

Narretz commented Feb 3, 2016

gkalpak Feb 3, 2016

gkalpak commented Feb 3, 2016

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

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

Conversation

Narretz commented Feb 2, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

thorn0 commented Feb 2, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Narretz commented Feb 2, 2016

gkalpak commented Feb 2, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Narretz commented Feb 3, 2016

Choose a reason for hiding this comment

gkalpak commented Feb 3, 2016