feat($compile): allow $watchCollection to be used in bi-directional bindings #9725

gabrielmaldi · 2014-10-21T13:29:11Z

Allow an asterisk to be specified in the binding definition to use $watchCollection instead of $watch.

scope: {
  prop: '=*'
}

This is a follow-up to #3491 but using $watchCollection as @IgorMinar suggested.

gkalpak · 2014-10-21T14:09:13Z

src/ng/compile.js

@@ -691,7 +693,7 @@ function $CompileProvider($provide, $$sanitizeUriProvider) {
  var EVENT_HANDLER_ATTR_REGEXP = /^(on[a-z]+|formaction)$/;

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


This RegExp will also match @* and &* which is unwanted.

... and when the bind is optional, it would be =?* that is very odd as it should be =*?

I also like the =*? syntax better. Will push fixes later 😄

mary-poppins · 2014-10-21T14:09:29Z

I'm sorry, but I wasn't able to verify your Contributor License Agreement (CLA) signature. CLA signature is required for any code contributions to AngularJS.

Please sign our CLA and ensure that the CLA signature email address and the email address in this PR's commits match.

If you signed the CLA as a corporation, please let us know the company's name.

Thanks a bunch!

PS: If you signed the CLA in the past then most likely the email addresses don't match. Please sign the CLA again or update the email address in the commit of this PR.
PS2: If you are a Googler, please sign the CLA as well to simplify the CLA verification process.

gkalpak · 2014-10-21T14:13:21Z

Shouldn't also the origin be updated if the local copy changes ?
Else that's not very bi-directional :)

gabrielmaldi · 2014-10-21T15:36:11Z

Shouldn't also the origin be updated if the local copy changes ?
Else that's not very bi-directional :)

It's working, parentValueWatch will update the origin scope. Or perhaps you implied that I didn't include a test for that 😄

gabrielmaldi · 2014-10-21T16:21:39Z

I modified the regexp and wrote a test to verify that the origin scope is updated when the isolate scope changes.

caitp · 2014-10-22T16:40:44Z

src/ng/compile.js

@@ -1894,7 +1897,9 @@ function $CompileProvider($provide, $$sanitizeUriProvider) {
                  return lastValue = parentValue;
                };
                parentValueWatch.$stateful = true;
-                var unwatch = scope.$watch($parse(attrs[attrName], parentValueWatch), null, parentGet.literal);
+                var unwatch = !definition.collection
+                    ? scope.$watch($parse(attrs[attrName], parentValueWatch), null, parentGet.literal)


can we do this with an if (definition.collection) { ... } else { ... } instead of the ternary operator? It's just a bit easier on the eyes

caitp · 2014-10-22T16:44:33Z

LGTM with nit

caitp · 2014-10-22T18:42:37Z

src/ng/compile.js

-                var unwatch = scope.$watch($parse(attrs[attrName], parentValueWatch), null, parentGet.literal);
+                var unwatch;
+                if (definition.collection) {
+                    unwatch = scope.$watchCollection(attrs[attrName], parentValueWatch);


indentation is wrong --- 2 spaces please

Oops, you're right. Sublime betrayed me, it usually detects how to indent based on the rest of the file 😄 Sorry.

gabrielmaldi · 2014-10-22T19:48:01Z

Are the tests failing due to changes in this PR? I'm guessing something external went wrong.

caitp · 2014-10-22T20:14:56Z

@gabrielmaldi something has been wrong with the unit tests all day, it could be that FF26 is just dead on SL right now. I'll try an experiment to see if it can work, but I am not an SL expert

caitp · 2014-10-22T20:24:40Z

hmm I don't think it's FF26 --- it's just that SL is dropping connections really badly... pretty sure that's what we're seeing.

gabrielmaldi · 2014-10-22T20:43:26Z

Thought so by examining the logs 😄

caitp · 2014-10-22T20:49:02Z

[2014-10-22 20:35:29.161] [DEBUG] launcher.sauce - Heartbeat to Sauce Labs (firefox 26) - fetching title
[2014-10-22 20:35:29.161] [DEBUG] wd -  > CALL title() 
[2014-10-22 20:35:29.162] [DEBUG] wd -  > GET /session/:sessionID/title 
[2014-10-22 20:35:37.438] [DEBUG] wd -  > RESPONSE get("about:blank") 
[2014-10-22 20:35:37.439] [DEBUG] wd -  > CALL quit() 
[2014-10-22 20:35:37.439] [DEBUG] wd -  > DELETE /session/:sessionID 
[2014-10-22 20:35:37.542] [DEBUG] wd -  > RESPONSE title() "Karma"
[2014-10-22 20:35:37.721] [DEBUG] wd - 
Ending your web drivage..

It seems to be just talking to FF wrong... but also FF27 when I tried it. I dunno =\

gabrielmaldi · 2014-10-23T13:18:41Z

@caitp good news, I amended to run the build again and SL seems to be back to normal 😄

And I did sign the CLA (at first I committed with a different email but it should validate now).

caitp · 2014-10-23T14:32:37Z

\o/

caitp · 2014-10-23T14:34:57Z

so I feel like should probably hear an lgtm from Igor, but it basically looks good to me --- however in your tests there is still some "off" indentation, I'll leave a comment

caitp · 2014-10-23T14:35:25Z

test/ng/compileSpec.js

+     }));
+
+     it('should update origin scope when isolate scope changes', inject(function() {
+        $rootScope.collection = [{


this entire block is 1 space further to the right than it should be (lines 3592 to 3609)

Done. And sorry for being so careless about indentation; I'm used to having code formatting automated on save in my own projects.

ah don't worry too hard about it, we all mess up formatting, even on projects that use clang-format in a git hook!

…indings an asterisk can be specified in the binding definition to use $watchCollection instead of $watch. e.g. scope: { prop: '=*' }

caitp · 2014-10-23T18:08:30Z

so, I'm just going to land this because it's doing what igor suggested and it basically looks okay to me --- I worry we might end up reverting it later though if someone decides it shouldn't have gone in.

Here goes, merg'in~

caitp · 2014-10-23T18:09:58Z

if this causes a regression, I'm gonna be sad :(

caitp · 2014-10-23T21:34:05Z

CLA verified by George Lu and Max Sills

gabrielmaldi · 2014-10-23T23:01:26Z

Great! Lets hope for no regressions and that Igor is fine with extending the syntax for this scenario. I know I'm gonna be waiting for the next release since I have many directives which take collections 😄

gkalpak · 2014-10-24T17:34:40Z

@caitp: Was this reverted or something ? Why don't I see the changes in the current snapshot ? :(

caitp · 2014-10-24T17:37:15Z

@gkalpak it wasn't reverted, but you're right, angularjs.org doesn't seem to be updated... maybe the jenkins machine died or got turned off or something

caitp · 2014-10-24T17:53:48Z

So, builds have been failing on the jenkins master branch, it probably gave up before trying to run the build update... that's been since the 20th... Unfortunately I can't recall the password for my account, and I'm pretty sure I don't have credentials to start it up again anyways... So, can see if it's fixed by pushing a build to master, but I think that's all we can do :(

the failures on jenkins look like e2e flakes to me, but there isn't much info there so I'm not sure.

gabrielmaldi mentioned this pull request Oct 21, 2014

Allow directive's bi-directional isolate scope to do deep equality check #3491

Closed

gabrielmaldi force-pushed the bi-directional-binding-watchCollection branch from d348a38 to 3c6690f Compare October 21, 2014 14:01

gkalpak reviewed Oct 21, 2014
View reviewed changes

mary-poppins added cla: no and removed cla: yes labels Oct 21, 2014

gabrielmaldi force-pushed the bi-directional-binding-watchCollection branch from 3c6690f to 6ce0643 Compare October 21, 2014 15:33

gabrielmaldi force-pushed the bi-directional-binding-watchCollection branch 2 times, most recently from 52c6435 to cc63f8a Compare October 21, 2014 16:20

caitp reviewed Oct 22, 2014
View reviewed changes

caitp added type: feature cla: maybe needs: merging labels Oct 22, 2014

caitp added this to the 1.3.x milestone Oct 22, 2014

caitp self-assigned this Oct 22, 2014

gabrielmaldi force-pushed the bi-directional-binding-watchCollection branch from cc63f8a to 5cbc9b2 Compare October 22, 2014 18:34

caitp reviewed Oct 22, 2014
View reviewed changes

gabrielmaldi force-pushed the bi-directional-binding-watchCollection branch 2 times, most recently from b592851 to df593c2 Compare October 22, 2014 19:16

gabrielmaldi force-pushed the bi-directional-binding-watchCollection branch from df593c2 to f759570 Compare October 23, 2014 13:00

gabrielmaldi force-pushed the bi-directional-binding-watchCollection branch from f759570 to a472ca7 Compare October 23, 2014 13:31

caitp reviewed Oct 23, 2014
View reviewed changes

feat($compile): allow $watchCollection to be used in bi-directional b…

06707a3

…indings an asterisk can be specified in the binding definition to use $watchCollection instead of $watch. e.g. scope: { prop: '=*' }

gabrielmaldi force-pushed the bi-directional-binding-watchCollection branch from a472ca7 to 06707a3 Compare October 23, 2014 15:26

caitp closed this in 40bbc98 Oct 23, 2014

caitp added cla: yes and removed cla: no cla: maybe labels Oct 23, 2014

gabrielmaldi deleted the bi-directional-binding-watchCollection branch February 13, 2015 17:40

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat($compile): allow $watchCollection to be used in bi-directional bindings #9725

feat($compile): allow $watchCollection to be used in bi-directional bindings #9725

gabrielmaldi commented Oct 21, 2014

gkalpak Oct 21, 2014

lgalfaso Oct 21, 2014

gabrielmaldi Oct 21, 2014

mary-poppins commented Oct 21, 2014

gkalpak commented Oct 21, 2014

gabrielmaldi commented Oct 21, 2014

gabrielmaldi commented Oct 21, 2014

caitp Oct 22, 2014

gabrielmaldi Oct 22, 2014

caitp commented Oct 22, 2014

caitp Oct 22, 2014

gabrielmaldi Oct 22, 2014

gabrielmaldi commented Oct 22, 2014

caitp commented Oct 22, 2014

caitp commented Oct 22, 2014

gabrielmaldi commented Oct 22, 2014

caitp commented Oct 22, 2014

gabrielmaldi commented Oct 23, 2014

caitp commented Oct 23, 2014

caitp commented Oct 23, 2014

caitp Oct 23, 2014

gabrielmaldi Oct 23, 2014

caitp Oct 23, 2014

caitp commented Oct 23, 2014

caitp commented Oct 23, 2014

caitp commented Oct 23, 2014

gabrielmaldi commented Oct 23, 2014

gkalpak commented Oct 24, 2014

caitp commented Oct 24, 2014

caitp commented Oct 24, 2014

feat($compile): allow $watchCollection to be used in bi-directional bindings #9725

feat($compile): allow $watchCollection to be used in bi-directional bindings #9725

Conversation

gabrielmaldi commented Oct 21, 2014

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mary-poppins commented Oct 21, 2014

gkalpak commented Oct 21, 2014

gabrielmaldi commented Oct 21, 2014

gabrielmaldi commented Oct 21, 2014

Choose a reason for hiding this comment

Choose a reason for hiding this comment

caitp commented Oct 22, 2014

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gabrielmaldi commented Oct 22, 2014

caitp commented Oct 22, 2014

caitp commented Oct 22, 2014

gabrielmaldi commented Oct 22, 2014

caitp commented Oct 22, 2014

gabrielmaldi commented Oct 23, 2014

caitp commented Oct 23, 2014

caitp commented Oct 23, 2014

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

caitp commented Oct 23, 2014

caitp commented Oct 23, 2014

caitp commented Oct 23, 2014

gabrielmaldi commented Oct 23, 2014

gkalpak commented Oct 24, 2014

caitp commented Oct 24, 2014

caitp commented Oct 24, 2014