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

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

Closed
wants to merge 1 commit into from
Closed
Show file tree
Hide file tree
Changes from all 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: 31 additions & 19 deletions src/ng/directive/ngPluralize.js
Original file line number Diff line number Diff line change
Expand Up @@ -173,7 +173,9 @@
</example>
*/
var ngPluralizeDirective = ['$locale', '$interpolate', function($locale, $interpolate) {
var BRACE = /{}/g;
var BRACE = /{}/g,
IS_WHEN = /^when(Minus)?(.+)$/;

return {
restrict: 'EA',
link: function(scope, element, attr) {
Expand All @@ -184,34 +186,44 @@ var ngPluralizeDirective = ['$locale', '$interpolate', function($locale, $interp
whensExpFns = {},
startSymbol = $interpolate.startSymbol(),
endSymbol = $interpolate.endSymbol(),
isWhen = /^when(Minus)?(.+)$/;
braceReplacement = startSymbol + numberExp + '-' + offset + endSymbol,
watchRemover = angular.noop,
lastCount;

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

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

maybe default to [] instead?

Copy link
Contributor

Choose a reason for hiding this comment

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

or {} I guess

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

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)

Copy link
Contributor

Choose a reason for hiding this comment

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

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

whensExpFns[key] =
$interpolate(expression.replace(BRACE, startSymbol + numberExp + '-' +
offset + endSymbol));
whensExpFns[key] = $interpolate(expression.replace(BRACE, braceReplacement));

});

scope.$watch(function ngPluralizeWatch() {
var value = parseFloat(scope.$eval(numberExp));
scope.$watch(numberExp, function ngPluralizeWatchAction(newVal) {
var count = parseFloat(newVal);
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 that using an interceptor here would be better as there will be no need to keep lastCount

Copy link
Member Author

Choose a reason for hiding this comment

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

I'll have to look into the interceptor concept; I am not familiar with it at all.

Copy link
Member Author

Choose a reason for hiding this comment

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

@lgalfaso: Hm...I could really use a hint here. How could be use an interceptor and how would an interceptor make lastCount unnecessary ?

The only thing that I could think of and would make lastCount unnecessary is moving part of the logic in the "watch" function instead of in the "action" function:

  var numberExpGetter = $parse(numberExp);
  scope.$watch(function ngPluralizeCountWatch() {
    var count = parseFloat(numberExpGetter(scope));
    if (!isNaN(count) && !(count in whens)) {
      count = $locale.pluralCat(count - offset);
    }
    return count;
  }, function ngPluralizeCountWatchAction(newVal) {
      watchRemover();
      watchRemover = scope.$watch(whensExpFns[newVal], updateElementText);
  });

But considering how "watch" functions should be as lightweight as possible, it seems clearly wrong to me to put an extra parseFloat + isNaN + (... in ...) + $locale.pluralCat in every $digest loop.

There is probably a way to let $parse know that ngPluralCountWatch depends on numberExpGetter(scope) (so it doesn't have to execute the rest of the logic if the "input" hasn't change), but like I said I could use a hint on how to achieve this through $watch.

Copy link
Contributor

Choose a reason for hiding this comment

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

You have to do scope.$watch($parse(expression, fn), ...);
where fn is ngPluralizeCountWatch

On Fri, Nov 14, 2014 at 11:45 AM, Georgios Kalpakas <
[email protected]> wrote:

In src/ng/directive/ngPluralize.js:

  •  scope.$watch(function ngPluralizeWatch() {
    
  •    var value = parseFloat(scope.$eval(numberExp));
    
  •  scope.$watch(numberExp, function ngPluralizeCountWatchAction(newVal) {
    
  •    var count = parseFloat(newVal);
    

@lgalfaso https://github.com/lgalfaso: Hm...I could really use a hint
here. How could be use an interceptor and how would an interceptor make
lastCount unnecessary ?

The only thing that I could think of and would make lastCount unnecessary
is moving part of the logic in the "watch" function instead of in the
"action" function:

var numberExpGetter = $parse(numberExp);
scope.$watch(function ngPluralizeCountWatch() {
var count = parseFloat(numberExpGetter(scope));
if (!isNaN(count) && !(count in whens)) {
count = $locale.pluralCat(count - offset);
}
return count;
}, function ngPluralizeCountWatchAction(newVal) {
watchRemover();
watchRemover = scope.$watch(whensExpFns[newVal], updateElementText);
});

But considering how "watch" functions should be as lightweight as
possible, it seems clearly wrong to me to put an extra parseFloat + isNaN

  • (... in ...) + $locale.pluralCat in every $digest loop.

There is probably a way to let $parse know that ngPluralCountWatch
depends on numberExpGetter(scope) (so it doesn't have to execute the rest
of the logic if the "input" hasn't change), but like I said I could use a
hint on how to achieve this through $watch.


Reply to this email directly or view it on GitHub
https://github.com/angular/angular.js/pull/10022/files#r20352580.

Copy link
Contributor

Choose a reason for hiding this comment

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

Sorry, I mean the function that converts the number into what at
ngPluralizeCountWatch
is called count

On Fri, Nov 14, 2014 at 6:16 PM, Lucas Mirelmann [email protected] wrote:

You have to do scope.$watch($parse(expression, fn), ...);
where fn is ngPluralizeCountWatch

On Fri, Nov 14, 2014 at 11:45 AM, Georgios Kalpakas <
[email protected]> wrote:

In src/ng/directive/ngPluralize.js:

  •  scope.$watch(function ngPluralizeWatch() {
    
  •    var value = parseFloat(scope.$eval(numberExp));
    
  •  scope.$watch(numberExp, function ngPluralizeCountWatchAction(newVal) {
    
  •    var count = parseFloat(newVal);
    

@lgalfaso https://github.com/lgalfaso: Hm...I could really use a hint
here. How could be use an interceptor and how would an interceptor make
lastCount unnecessary ?

The only thing that I could think of and would make lastCount
unnecessary is moving part of the logic in the "watch" function instead of
in the "action" function:

var numberExpGetter = $parse(numberExp);
scope.$watch(function ngPluralizeCountWatch() {
var count = parseFloat(numberExpGetter(scope));
if (!isNaN(count) && !(count in whens)) {
count = $locale.pluralCat(count - offset);
}
return count;
}, function ngPluralizeCountWatchAction(newVal) {
watchRemover();
watchRemover = scope.$watch(whensExpFns[newVal], updateElementText);
});

But considering how "watch" functions should be as lightweight as
possible, it seems clearly wrong to me to put an extra parseFloat +
isNaN + (... in ...) + $locale.pluralCat in every $digest loop.

There is probably a way to let $parse know that ngPluralCountWatch
depends on numberExpGetter(scope) (so it doesn't have to execute the
rest of the logic if the "input" hasn't change), but like I said I could
use a hint on how to achieve this through $watch.


Reply to this email directly or view it on GitHub
https://github.com/angular/angular.js/pull/10022/files#r20352580.

var countIsNaN = isNaN(count);

if (!isNaN(value)) {
//if explicit number rule such as 1, 2, 3... is defined, just use it. Otherwise,
//check it against pluralization rules in $locale service
if (!(value in whens)) value = $locale.pluralCat(value - offset);
return whensExpFns[value](scope);
} else {
return '';
if (!countIsNaN && !(count in whens)) {
// If an explicit number rule such as 1, 2, 3... is defined, just use it.
// Otherwise, check it against pluralization rules in $locale service.
count = $locale.pluralCat(count - offset);
}

// If both `count` and `lastCount` are NaN, we don't need to re-register a watch.
// In JS `NaN !== NaN`, so we have to exlicitly check.
if ((count !== lastCount) && !(countIsNaN && isNaN(lastCount))) {
watchRemover();
watchRemover = scope.$watch(whensExpFns[count], updateElementText);
lastCount = count;
}
}, function ngPluralizeWatchAction(newVal) {
element.text(newVal);
});

function updateElementText(newText) {
element.text(newText || '');
}
}
};
}];
72 changes: 72 additions & 0 deletions test/ng/directive/ngPluralizeSpec.js
Original file line number Diff line number Diff line change
Expand Up @@ -242,4 +242,76 @@ describe('ngPluralize', function() {
});
});
});


describe('bind-once', function() {

it('should support for `count` to be a one-time expression',
inject(function($compile, $rootScope) {
element = $compile(
'<ng:pluralize count="::email"' +
"when=\"{'one': 'You have one new email'," +
"'other': 'You have {} new emails'}\">" +
'</ng:pluralize>')($rootScope);
elementAlt = $compile(
'<ng:pluralize count="::email" ' +
"when-one='You have one new email' " +
"when-other='You have {} new emails'>" +
'</ng:pluralize>')($rootScope);

$rootScope.email = undefined;
$rootScope.$digest();
expect(element.text()).toBe('');
expect(elementAlt.text()).toBe('');

$rootScope.email = 3;
$rootScope.$digest();
expect(element.text()).toBe('You have 3 new emails');
expect(elementAlt.text()).toBe('You have 3 new emails');

$rootScope.email = 2;
$rootScope.$digest();
expect(element.text()).toBe('You have 3 new emails');
expect(elementAlt.text()).toBe('You have 3 new emails');

$rootScope.email = 1;
$rootScope.$digest();
expect(element.text()).toBe('You have 3 new emails');
expect(elementAlt.text()).toBe('You have 3 new emails');
})
);


it('should still update other embedded expressions',
inject(function($compile, $rootScope) {
element = $compile(
'<ng:pluralize count="::email"' +
"when=\"{'one': 'You, {{user}}, have one new email'," +
"'other': 'You, {{user}}, have {} new emails'}\">" +
'</ng:pluralize>')($rootScope);
elementAlt = $compile(
'<ng:pluralize count="::email" ' +
"when-one='You, {{user}}, have one new email' " +
"when-other='You, {{user}}, have {} new emails'>" +
'</ng:pluralize>')($rootScope);

$rootScope.user = 'Lucas';
$rootScope.email = undefined;
$rootScope.$digest();
expect(element.text()).toBe('');
expect(elementAlt.text()).toBe('');

$rootScope.email = 3;
$rootScope.$digest();
expect(element.text()).toBe('You, Lucas, have 3 new emails');
expect(elementAlt.text()).toBe('You, Lucas, have 3 new emails');

$rootScope.user = 'Pete';
$rootScope.email = 2;
$rootScope.$digest();
expect(element.text()).toBe('You, Pete, have 3 new emails');
expect(elementAlt.text()).toBe('You, Pete, have 3 new emails');
})
);
});
});