fix(core): drop the toBoolean function #7960

mgol · 2014-06-23T22:08:11Z

So far Angular have used the toBoolean function to decide if the parsed value
is truthy. The function made more values falsy than regular JavaScript would,
e.g. strings 'f' and 'no' were both treated as falsy. This creates suble bugs
when backend sends a non-empty string with one of these values and something
suddenly hides in the application

BREAKING CHANGE: values 'f', '0', 'false', 'no', 'n', '[]' are no longer
treated as falsy. Only JavaScript falsy values are now treated as falsy by the
expression parser; there are six of them: false, null, undefined, NaN, 0 and "".

Fixes #3969
Fixes #4277

cc @IgorMinar @petebacondarwin

mary-poppins · 2014-06-23T22:08:14Z

Thanks for the PR! Please check the items below to help us merge this faster. See the contributing docs for more information.

Uses the issue template (#7960)

If you need to make changes to your pull request, you can update the commit with git commit --amend.
Then, update the pull request with git push -f.

Thanks again for your help!

This issue has been a focus of problems for some users and we discussed it on the IRC that it should be at least documented. ~Amended the style to use bootstrap notes, I think overall it looks better and catches the eyes more easily. However there are no anchor links to these, if these are necessary they can be added later. Closes #3436 Closes #5762

caitp · 2014-06-23T22:45:34Z

src/ng/directive/ngIf.js

@@ -87,7 +87,7 @@ var ngIfDirective = ['$animate', function($animate) {
        var block, childScope, previousElements;
        $scope.$watch($attr.ngIf, function ngIfWatchAction(value) {

-          if (toBoolean(value)) {
+          if (!!value) {


the !! is unnecessary here, truthy is truthy regardless

Yeah, sorry for that.

caitp · 2014-06-23T22:50:35Z

I think you probably want some tests for the other areas affected by the change, such as ngIf and ngAnimate, but that's just me

caitp · 2014-06-24T01:17:27Z

I'm also not sure this really adds anything amazing over the closed PRs

IgorMinar · 2014-06-24T05:11:38Z

This change looks good to me except for the unnecessary casts to boolean.

Please add a test that verifies that ng-if="'no'" is treated as truthy and you can go ahead and merge this in.

lgalfaso · 2014-06-24T06:44:55Z

src/ng/directive/input.js

@@ -917,7 +917,7 @@ function textInputType(scope, element, attr, ctrl, $sniffer, $browser) {
    // By default we will trim the value
    // If the attribute ng-trim exists we will avoid trimming
    // e.g. <input ng-model="foo" ng-trim="false">
-    if (toBoolean(attr.ngTrim || 'T')) {
+    if (!attr.ngTrim || attr.ngTrim !== 'false') {


Why not just if (attr.ngTrim !== 'false') { ?

This catches the case where attr.ngTrim === false as well as attr.ngTrim === 'false'

it's not clear that we really want to test for it to equal 'false' if the point of this PR is to get rid of toBoolean-like behaviour, though. Why not just !attr.ngTrim? I guess we aren't parsing it, hmm.

Ah, sorry for the noise

Actually, it catches both when attr.ngTrim === undefined (not false) and when attr.ngTrim === 'false'. attr.ngTrim is either undefined or a string, like all attr values.

So yeah, we're not parsing it (I don't know why), so we have to sort-of parse it here.

Btw, that also means ng-trim="some_falsy_variable" won't work, why is it implemented in this way?

cost of watching, probably

we just assumed that this is a "static" setting so we didn't want to complicate the implementation with parsing and full expression support.

we should consider moving this to ngModelOptions now that we have it. but that's for a separate PR.

@petebacondarwin what do you think?

I agree @IgorMinar - it is a good candidate for ngModelOptions. @shahata - perhaps another one of your nice PRs?

mgol · 2014-06-24T14:01:52Z

@caitp

I'm also not sure this really adds anything amazing over the closed PRs

Nothing amazing here. ;) But as for PRs, #7776 just adds docs about this weird behavior to ngIf and doesn't remove it so it's different; I guess I could've asked @lgalfaso to submit a new PR based on his #4005 that would do all of that.

Maybe I'll incorporate tests from @lgalfaso to a separate commit after merging this one (I'd still modify them a bit; now that I looked at it, ngIf tests use the makeIf helper and @lgalfaso's tests don't; that would simplify the code)? I don't want to step on anyone's toes or discourage anyone.

lgalfaso · 2014-06-24T14:15:26Z

@mzgol this is a good PR and if the end result is good, I do not find it important who gets the credit on a specific feature. That aside, it would be nice if some of the tests from #4005 were added, but given that the logic is much simpler, then I am ok with the PR as is

mgol · 2014-06-24T14:29:23Z

@lgalfaso Thanks. I added wrapping in !!( ) as you did in #4005 since after porting your tests to be more in line with current ones I had an infinite digest; that's because [] !== [], though I had an impression that there was a patch to Angular some time ago that made such cases allocate once, preventing infinite digest. Did I remember sth wrong? @IgorMinar

mgol · 2014-06-24T15:10:14Z

@IgorMinar @caitp @lgalfaso PR updated. I added more tests and changed some other needed things so please review again.

IgorMinar · 2014-06-24T15:51:58Z

src/ng/directive/ngIf.js

@@ -85,9 +85,9 @@ var ngIfDirective = ['$animate', function($animate) {
    $$tlb: true,
    link: function ($scope, $element, $attr, ctrl, $transclude) {
        var block, childScope, previousElements;
-        $scope.$watch($attr.ngIf, function ngIfWatchAction(value) {
+        $scope.$watch('!!(' + $attr.ngIf + ')', function ngIfWatchAction(value) {


this doesn't look right to me. we should be able to handle even [] or {} without infinite loop.

ahh... I see. when we added one-time binding, we broke this because the constants are not unwatched after the digest loop which is too late and that causes infinite digest. cc: @lgalfaso @rodyhaddad

I agree, this does not look right. Let me look into this

Ok, it looks like this is a regression from the one-time bind implementation. #7970 should fix this to the point that this PR can move forward and until #7700 is iron out

OK, so I guess I'll wait for #7970 and then remove this wrapping? This does cause the watcher to fire when changing from a truthy value to another truthy value; is there any performance difference between evaluating:

$scope.$watch('foo', handler);

and

$scope.$watch('!!foo', handler);

?

"Adding" an existing class should be a noop, so the cost is mainly due to invoking the watch handler; if changing a simple field to an expression adds new functions to the pipeline, there should be no perf difference.

IgorMinar · 2014-06-25T21:27:35Z

I asked pete to merge #7970 today so this one is unblocked.

mgol · 2014-06-25T21:35:03Z

@IgorMinar

I asked pete to merge #7970 today so this one is unblocked.

So I should revert the !!( ) wrapping? See #7960 (comment)

So far Angular have used the toBoolean function to decide if the parsed value is truthy. The function made more values falsy than regular JavaScript would, e.g. strings 'f' and 'no' were both treated as falsy. This creates suble bugs when backend sends a non-empty string with one of these values and something suddenly hides in the application Thanks to lgalfaso for test ideas. BREAKING CHANGE: values 'f', '0', 'false', 'no', 'n', '[]' are no longer treated as falsy. Only JavaScript falsy values are now treated as falsy by the expression parser; there are six of them: false, null, undefined, NaN, 0 and "". Fixes angular#3969 Fixes angular#4277

mgol · 2014-06-25T21:53:38Z

@IgorMinar I rebased (#7970 was merged) and removed !!( ) wrapping for now, let me know what you think.

IgorMinar · 2014-06-26T14:30:35Z

looks great to me

IgorMinar · 2014-06-27T04:23:42Z

Awesome. Thanks!
On Jun 26, 2014 12:52 PM, "Michał Gołębiowski" notifications@github.com
wrote:

Closed #7960 #7960 via bdfc9c0
bdfc9c0
.

—
Reply to this email directly or view it on GitHub
#7960 (comment).

So far Angular have used the toBoolean function to decide if the parsed value is truthy. The function made more values falsy than regular JavaScript would, e.g. strings 'f' and 'no' were both treated as falsy. This creates suble bugs when backend sends a non-empty string with one of these values and something suddenly hides in the application Thanks to lgalfaso for test ideas. BREAKING CHANGE: values 'f', '0', 'false', 'no', 'n', '[]' are no longer treated as falsy. Only JavaScript falsy values are now treated as falsy by the expression parser; there are six of them: false, null, undefined, NaN, 0 and "". Closes angular#3969 Closes angular#4277 Closes angular#7960

This was referenced Jun 23, 2014

fix(ng-if): make ng-if evaluate things using javascript logic #4005

Closed

Update ngIf.js doc. Added missing description of ng-if behavior. #7776

Closed

mgol added this to the 1.3.0 milestone Jun 23, 2014

mgol added cla: yes and removed cla: no labels Jun 23, 2014

mgol modified the milestones: 1.3.0-beta.14, 1.3.0 Jun 23, 2014

caitp reviewed Jun 23, 2014
View reviewed changes

lgalfaso reviewed Jun 24, 2014
View reviewed changes

IgorMinar reviewed Jun 24, 2014
View reviewed changes

Narretz mentioned this pull request Jun 25, 2014

ng-if always evaluates to false if first variable is an empty array #7989

Closed

mgol closed this in bdfc9c0 Jun 26, 2014

mgol deleted the toBoolean branch June 26, 2014 21:00

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(core): drop the toBoolean function #7960

fix(core): drop the toBoolean function #7960

mgol commented Jun 23, 2014

mary-poppins commented Jun 23, 2014

caitp Jun 23, 2014

IgorMinar Jun 24, 2014

mgol Jun 24, 2014

caitp commented Jun 23, 2014

caitp commented Jun 24, 2014

IgorMinar commented Jun 24, 2014

lgalfaso Jun 24, 2014

petebacondarwin Jun 24, 2014

caitp Jun 24, 2014

lgalfaso Jun 24, 2014

mgol Jun 24, 2014

caitp Jun 24, 2014

IgorMinar Jun 24, 2014

IgorMinar Jun 24, 2014

petebacondarwin Jun 26, 2014

mgol commented Jun 24, 2014

lgalfaso commented Jun 24, 2014

mgol commented Jun 24, 2014

mgol commented Jun 24, 2014

IgorMinar Jun 24, 2014

lgalfaso Jun 24, 2014

lgalfaso Jun 24, 2014

mgol Jun 24, 2014

IgorMinar commented Jun 25, 2014

mgol commented Jun 25, 2014

mgol commented Jun 25, 2014

IgorMinar commented Jun 26, 2014

IgorMinar commented Jun 27, 2014

fix(core): drop the toBoolean function #7960

fix(core): drop the toBoolean function #7960

Conversation

mgol commented Jun 23, 2014

mary-poppins commented Jun 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 Jun 23, 2014

caitp commented Jun 24, 2014

IgorMinar commented Jun 24, 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mgol commented Jun 24, 2014

lgalfaso commented Jun 24, 2014

mgol commented Jun 24, 2014

mgol commented Jun 24, 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

IgorMinar commented Jun 25, 2014

mgol commented Jun 25, 2014

mgol commented Jun 25, 2014

IgorMinar commented Jun 26, 2014

IgorMinar commented Jun 27, 2014