feat(ngPluralize): add support for bind-once in `count` #10022

gkalpak · 2014-11-12T11:58:29Z

The 1st commit contains the change and test for adding support for bindOnce in count.
The 2nd commit contains some minor refactoring (mainly to avoid repetitive actions).

Closes #10004
(Makes #5452 obsolete)

caitp · 2014-11-12T16:43:34Z

src/ng/directive/ngPluralize.js

+        tmpMatch = IS_WHEN.exec(attributeName);
+        if (tmpMatch) {
+          var whenKey = lowercase((tmpMatch[1] ? '-' : '') + tmpMatch[2]);
+          whens[whenKey] = element.attr(attr.$attr[attributeName]);
        }
      });
      forEach(whens, function(expression, key) {


if whens was created with createMap() then forEach() will break :(

maybe default to [] instead?

or {} I guess

alternatively, you could remove the forEach() and just do it Object.keys()-for-loop style.

True !
But shouldn't we rather be fixing forEach() ?
(I.e. replacing obj.hasOwnProperty(...) with hasOwnProperty.call(obj, ...) at https://github.com/angular/angular.js/blob/master/src/Angular.js#L265)

i don't care a whole lot about that --- @petebacondarwin would you be pro-refactoring forEach?

caitp · 2014-11-12T18:43:42Z

@lgalfaso since you wanted #10004 you can review this =)

jbedard · 2014-11-13T07:09:11Z

I was about to say that this solution looks way too confusing - but it looks like you fixed it up as I started typing. Here's what I tried (yours basically does the same now): jbedard@6ffe579

gkalpak · 2014-11-13T07:11:58Z

@lgalfaso: I addressed your comments (and squashed everything in a single commit).

gkalpak · 2014-11-13T07:12:57Z

@jbedard: Actually, there is a problem with both our imlementation:
If a when expression has other embedded expressions (e.g. Hello, {{user}}, you have {} unread emails.), the element's text will not reflect any new values of them (e.g. user), because the watch only watches over count.
(Working on it.)

jbedard · 2014-11-13T07:23:41Z

Interesting... that makes it way more complicated :|

jbedard · 2014-11-13T07:32:27Z

Could watch numberExp (like we both did), and when it changes watch the appropriate interpolation (and unwatch the old one). As long as calling watch/unwatch whenever the number changes isn't a perf hit. Then, now that the result of $interpolate() will be watched directly, it will allow the $$watchDelegate to be used and will avoid doing an expensive $interpolate per-digest...

jbedard · 2014-11-13T07:39:21Z

Seems to work: jbedard@1583415, haven't looked into the performance differences at all though (I think it will actually be faster by not doing the interpolation in the watcher though).

gkalpak · 2014-11-13T08:09:48Z

@jbedard: That was my first thought too. One problem is that the count will also be updated, since in whensExpFns we have replaced {} with {{<numberExp> - <offset>}}.

jbedard · 2014-11-13T08:19:37Z

But that {} should be replaced with the bind-once numberExp. So assuming it doesn't change again between the numberExp watcher being invoked and the first interpolation watcher being invoked it should work? I think?

gkalpak · 2014-11-13T08:46:18Z

I pushed a 2nd commit which seems to fix the issue (in a different way than @jbedard).
Still not 100% satisfied with it.

Will play a bit with jbedard@1583415

gkalpak · 2014-11-13T09:02:19Z

But that {} should be replaced with the bind-once numberExp. So assuming it doesn't change again between the numberExp watcher being invoked and the first interpolation watcher being invoked it should work.

You are right about that.

Overall, I think I like the 2nd watcher approach better - perf implications aside (if any).
It might be better to not remove/readd the $watcher at every $digest, but just change the value somehwere it can access it.

@lgalfaso: WDYT ?

gkalpak · 2014-11-13T09:36:13Z

It might be better to not remove/readd the $watcher at every $digest, but just change the value somehwere it can access it.

Seems like we can't do that. (I guess I am not familiar enough with $interpolate and the $$watchDelegate thingy.)

gkalpak · 2014-11-13T09:55:28Z

I pushed another commit using the 2 watchers approach (similar to @jbedard's jbedard@1583415) and I am pretty happy with it 😃.

I added a check, so that we only unregister and re-register the 2nd watcher only if the key did actually change (e.g. the count might change from 10 to 20, but the watched function is still whensExpFns['other'], so we don't need to re-register).

/ping @lgalfaso

_{If it lgty, let me know so I squash the commits into 1.}

lgalfaso · 2014-11-13T21:24:21Z

@gkalpak I do not feel comfortable with how this is going. If the idea is to fix #10004, then the PR should be about this. Just trying to bundle everything that can be done to ngPluralize into a single PR does not make things any easier... specially when this change involves a breaking change

I would like to have a single commit that the only thing it does is fix the valid use case of #10004 (some small cleanup would be ok, but not behavior changes)

gkalpak · 2014-11-14T08:05:10Z

@lgalfaso: Not sure what behaviour changes you are talking about. The changes in this PR are about adding support for count to be a one-time expression (+ some minor cleanup).
Unfortunately, due to the previous implementation of ngPluralize, adding that kind of support isn't as straightforward.

Feel free to suggest a simpler approach; I might have missed something.

_{Also, not sure what is the breaking change you mention (other than that one-time expressions will work with `count`)...}

lgalfaso · 2014-11-14T08:19:12Z

src/ng/directive/ngPluralize.js

-          $interpolate(expression.replace(BRACE, startSymbol + numberExp + '-' +
-            offset + endSymbol));
-      });
+      for (var key in whens) {


I find it cleaner to use forEach even when this implies that we end up using a {} or Object.keys()

I really wonder how forEach is cleaner, but I will change it 😃

lgalfaso · 2014-11-14T08:19:57Z

src/ng/directive/ngPluralize.js

+        if (count !== lastCount) {
+          watchRemover();
+          watchRemover = scope.$watch(whensExpFns[count],
+            function ngPluralizeWhensWatchAction(newVal) {


please move this function out, as it uses nothing from the closure

lgalfaso · 2014-11-14T08:28:23Z

sorry, I misread part of the patch

once the changes from the comment are in place, the PR LGTM

gkalpak · 2014-11-14T21:21:20Z

@lgalfaso: Thx for the help on watch interceptors. This works great 😃
I addressed your comments and rebased.

caitp · 2014-11-14T21:26:19Z

src/ng/directive/ngPluralize.js

+        watchRemover = scope.$watch(whensExpFns[newVal], updateElementText);
+      }
+
+      function ngPluralizeWatchInterceptor(newVal) {


if at all possible, can we try to do this without interceptors? Otherwise it makes it slightly harder to get rid of them :(

your call @lgalfaso but it would be better

The previous implementation was without interceptors (but used an extra local variable).
So, it is doable (remains to see if it is desirable).

@caitp: Why remove interceptors ? They seem pretty cool :) What's the catch ?

they add an extra hook in an aleady multi-hook process which complicates things and causes headaches which are very hard to debug, I want them gone .v.

after talking with @caitp I find it clear that it is better not to use the interceptor. Sorry @gkalpak for making you work extra, but can you repost this PR with a version that does not use an interceptor?
Thanks!

I also want them gone... but they do provide a useful feature of filtering the output of an expression like what @lgalfaso mentioned. But I don't like how they are implemented right now, but also haven't thought of anything better yet.

gkalpak · 2014-11-14T23:27:01Z

Back to not using $parse interceptors.

lgalfaso · 2014-11-15T01:10:13Z

LGTM
will merge after we cut 1.3.3

Closes angular#10004

gkalpak · 2014-11-18T11:05:47Z

(FYI: I just removed an unused $parse dependency - a left-over from rapidly switching between interceptor/no-interceptor implementation 😃)

lgalfaso · 2014-11-18T22:50:03Z

landed 2b41a58

gkalpak force-pushed the ngPluralize-bindOnce-count branch from d601008 to a3218a3 Compare November 12, 2014 12:03

gkalpak mentioned this pull request Nov 12, 2014

Support one-time binding in ng-pluralize #10004

Closed

gkalpak force-pushed the ngPluralize-bindOnce-count branch from a3218a3 to a469b46 Compare November 12, 2014 12:09

mary-poppins added cla: yes and removed cla: no labels Nov 12, 2014

caitp reviewed Nov 12, 2014
View reviewed changes

caitp added component: misc core type: feature severity: broken expected use labels Nov 12, 2014

caitp added this to the 1.3.4 milestone Nov 12, 2014

caitp assigned lgalfaso Nov 12, 2014

gkalpak force-pushed the ngPluralize-bindOnce-count branch 2 times, most recently from 2908731 to ff6f155 Compare November 13, 2014 06:55

gkalpak force-pushed the ngPluralize-bindOnce-count branch from 1e1c080 to abc03a3 Compare November 13, 2014 08:49

lgalfaso reviewed Nov 14, 2014
View reviewed changes

gkalpak force-pushed the ngPluralize-bindOnce-count branch 2 times, most recently from d388a4a to 67c88b7 Compare November 14, 2014 21:19

caitp reviewed Nov 14, 2014
View reviewed changes

gkalpak force-pushed the ngPluralize-bindOnce-count branch from 67c88b7 to 8a24974 Compare November 14, 2014 23:23

gkalpak force-pushed the ngPluralize-bindOnce-count branch from 8a24974 to a6029ee Compare November 18, 2014 09:53

feat(ngPluralize): add support for count to be a one-time expression

e4dee6a

Closes angular#10004

gkalpak force-pushed the ngPluralize-bindOnce-count branch from a6029ee to e4dee6a Compare November 18, 2014 11:03

lgalfaso closed this Nov 18, 2014

gkalpak deleted the ngPluralize-bindOnce-count branch November 18, 2014 23:00

This was referenced Nov 23, 2014

refactor(ng-pluralize): simplify the isWhen regexp #5452

Closed

ngPluralize throws "TypeError: undefined is not a function" when including locale file #7774

Closed

feat(ngPluralize): add support for bind-once in count #10022

feat(ngPluralize): add support for bind-once in count #10022

Conversation

gkalpak commented Nov 12, 2014

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

caitp commented Nov 12, 2014

jbedard commented Nov 13, 2014

gkalpak commented Nov 13, 2014

gkalpak commented Nov 13, 2014

jbedard commented Nov 13, 2014

jbedard commented Nov 13, 2014

jbedard commented Nov 13, 2014

gkalpak commented Nov 13, 2014

jbedard commented Nov 13, 2014

gkalpak commented Nov 13, 2014

gkalpak commented Nov 13, 2014

gkalpak commented Nov 13, 2014

gkalpak commented Nov 13, 2014

lgalfaso commented Nov 13, 2014

gkalpak commented Nov 14, 2014

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lgalfaso commented Nov 14, 2014

gkalpak commented Nov 14, 2014

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

gkalpak commented Nov 14, 2014

lgalfaso commented Nov 15, 2014

gkalpak commented Nov 18, 2014

lgalfaso commented Nov 18, 2014

feat(ngPluralize): add support for bind-once in `count` #10022

feat(ngPluralize): add support for bind-once in `count` #10022