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

lgalfaso · 2013-09-13T18:57:41Z

Make ng-if evaluate things using javascript logic
Do not reevaluate everything contained within an ng-if when there
is a change of the value of the ng-if, but the truthy of falsy value
does not change

Fixes #3969

BREAKING CHANGE: The expressions

<div ng-if="[]">X</div>
<div ng-if="'f'">X</div>
<div ng-if="'[]'">X</div>

used to be evaluated to false

mary-poppins · 2013-09-13T18:57:44Z

Thanks for the PR!

Contributor signed CLA now or in the past
- If you just signed, leave a comment here with your real name
PR's commit messages follow the commit message format

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!

petebacondarwin · 2013-09-13T20:44:13Z

src/ng/directive/ngIf.js

@@ -86,7 +86,7 @@ var ngIfDirective = ['$animate', function($animate) {
    compile: function (element, attr, transclude) {
      return function ($scope, $element, $attr) {
        var childElement, childScope;
-        $scope.$watch($attr.ngIf, function ngIfWatchAction(value) {
+        $scope.$watch('!!(' + $attr.ngIf + ')', function ngIfWatchAction(value) {


It would seem to me cleaner to do something like:

$scope.$watch($attr.ngIf, function ngIfWatchAction(value) { ... if (!!value) { ... } ... });

Oh, I guess that if the value changes but it is still true we don't want to delete the childElement.

Yep, that was the idea

mgol · 2013-10-17T20:01:31Z

@petebacondarwin It would be better to get this merged for 1.2.0 than 1.2.1 as in this case you'd introduce a breaking change in a patch version which is not the best idea...

caitp · 2014-02-05T18:00:51Z

@lgalfaso could you rebase this? It would be good to get this into 1.3 --- And similar to the ngShowHide changes, please included a short summary of how affected users could migrate in the BREAKING CHANGE block

lgalfaso · 2014-02-05T22:13:45Z

@caitp 4e4c9e9 is a rebase with the changes for ng-if, ng-show and ng-hide

caitp · 2014-02-05T22:20:45Z

Yeah thanks =) Were you going to close your other PR then, since this one contains both fixes? (I think it would be better to have kept the ngShowHide changes out of this one, but well...)

lgalfaso · 2014-02-05T22:25:04Z

@caitp if you think it would be best to keep these two as different PRs, then that should not be an issue. I put these two together just to be consistent

mgol · 2014-05-12T23:31:09Z

Won't watching '!!(' + attr.ngShow + ')' instead of just attr.ngShow make it harder to optimize it via passing to Object.observe? I guess we don't add support for O.o in 1.3 anyway so that's not an issue, @caitp?

I'd like to merge it rather soonish since it does change semantics so it's better to prepare people for that as early as possible.

mgol · 2014-05-12T23:43:44Z

Actually, shouldn't we just fix toBoolean once and for all? I mean, this whole function:
https://github.com/angular/angular.js/blob/master/src/Angular.js#L1001-L1011
should be removed and its uses replaced with regular !! where needed.

lgalfaso · 2014-05-13T00:14:26Z

@mzgol doing '!!(' + attr.ngShow + ')' has the added benefit that if the value of attr.ngShow changes from one truthy value to another truthy value, then the watch is not called. BTW, +1 on fixing toBoolean

mgol · 2014-05-13T00:24:56Z

@lgalfaso

doing '!!(' + attr.ngShow + ')' has the added benefit that if the value of attr.ngShow changes from one truthy value to another truthy value, then the watch is not called.

I know, I just mentioned it because if we had Object.observe then passing a regular reference would make it not checked on every $digest which is required when you modify it on the fly so this approach would be bad in Angular 2. But here it may be better because of what you said. Context matters. :)

Make ngIf, ngShow and ngHide follow javascript `truthy`/`falsy` logic and not the custom toBoolean logic Fixes angular#5414 angular#4277 angular#3969 BREAKING CHANGE: The expressions * `<div ng-hide="[]">X</div>` * `<div ng-hide="'f'">X</div>` * `<div ng-hide="'[]'">X</div>` used to be evaluated to `false` and the elements were hidden. The same effect is present for `ng-show` and the elements are now visible; and with `ng-if` and the elements are now removed If you were previously doing `ng-show="exp"` where `$scope.exp = 'no' // (or 'n' or 'f')`, then instead write `ng-show="exp && exp !== 'no'` (or 'n' or 'f').

petebacondarwin · 2014-06-20T11:16:02Z

Regarding '!!(' + attr.ngShow + ')' I think it is better to check the newValue against the oldValue in the watch handler. Yes, it does mean that the watch handler gets called a few more times if the value goes from say, '' to 0. But equally we are not having to do the !! operation on every watch function.

mgol · 2014-06-20T11:18:40Z

@petebacondarwin I'll be getting rid of toBoolean in a separate PR soon so this will probably be obsolete.

petebacondarwin · 2014-06-20T11:33:46Z

Here are the places where toBoolean is used and how we should deal with them. Then we can remove toBoolean altogether.

For ngIf, ngShow and ngHide we should, as you say, remove the call to toBoolean and use !! instead, causing a breaking change.
Where toBoolean is used in input for ngTrim we should put in place specific functionality and only check whether the value of the attribute is the string "false". This would also be a breaking change.
Where toBoolean is used in the orderBy filter, we are guaranteed to only have a boolean value anyway so this should simply be removed, with no breaking change.

petebacondarwin · 2014-06-20T11:35:04Z

@mzgol - are you dealing with the items I specified above then?

mgol · 2014-06-20T12:03:45Z

@petebacondarwin Yes, I'll go through every usage and see what needs to be done to fully replace it. Thanks for your analysis, it'll help.

petebacondarwin · 2014-06-20T12:12:14Z

Assigning to @mzgol then!

IgorMinar · 2014-06-22T08:49:09Z

Thanks @petebacondarwin for looking into this one.

@mzgol will you be able to get this done by Monday or Tuesday? If not, @petebacondarwin or someone else will take care of it.

mgol · 2014-06-22T10:10:52Z

I'll look into it tomorrow.

Michał Gołębiowski

IgorMinar · 2014-06-23T00:35:06Z

Great! Thanks
On Jun 22, 2014 3:11 AM, "Michał Gołębiowski" notifications@github.com
wrote:

I'll look into it tomorrow.

Michał Gołębiowski

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

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

Closing in favor of #7960.

petebacondarwin reviewed Sep 13, 2013
View reviewed changes

lgalfaso mentioned this pull request Dec 31, 2013

fix(ngShow/ngHide): follow javscript truthy/falsy logic #5573

Closed

fmalk mentioned this pull request Jan 8, 2014

ng-hide evaluates empty strings as truthy #5691

Closed

caitp added gh: PR labels Feb 5, 2014

caitp modified the milestones: 1.3.x, Backlog Feb 5, 2014

caitp mentioned this pull request Mar 25, 2014

ng-if doesn't work as expected #6835

Closed

lgalfaso mentioned this pull request Apr 14, 2014

[false] and [0] being treated as falsy in ngShow #7110

Closed

lgalfaso mentioned this pull request Apr 28, 2014

docs(ngIf): add warning regarding falsy values #7293

Closed

Narretz mentioned this pull request May 1, 2014

ng-show expression evaluates to false when passed "f" or "F" #7312

Closed

lgalfaso mentioned this pull request Jun 5, 2014

ngIf and array values #7708

Closed

IgorMinar added the high priority label Jun 5, 2014

lgalfaso mentioned this pull request Jun 13, 2014

Inconsistent ng-if behaviour with strings. #7742

Closed

petebacondarwin self-assigned this Jun 16, 2014

petebacondarwin added this to the 1.3.0-beta.13 milestone Jun 16, 2014

petebacondarwin removed this from the 1.3.0 milestone Jun 16, 2014

petebacondarwin assigned mgol and unassigned petebacondarwin Jun 20, 2014

petebacondarwin modified the milestones: 1.3.0-beta.14, 1.3.0-beta.13 Jun 20, 2014

mgol closed this Jun 23, 2014

mgol mentioned this pull request Jun 24, 2014

fix(core): drop the toBoolean function #7960

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

lgalfaso commented Sep 13, 2013

mary-poppins commented Sep 13, 2013

petebacondarwin Sep 13, 2013

petebacondarwin Sep 13, 2013

lgalfaso Sep 13, 2013

mgol commented Oct 17, 2013

caitp commented Feb 5, 2014

lgalfaso commented Feb 5, 2014

caitp commented Feb 5, 2014

lgalfaso commented Feb 5, 2014

mgol commented May 12, 2014

mgol commented May 12, 2014

lgalfaso commented May 13, 2014

mgol commented May 13, 2014

petebacondarwin commented Jun 20, 2014

mgol commented Jun 20, 2014

petebacondarwin commented Jun 20, 2014

petebacondarwin commented Jun 20, 2014

mgol commented Jun 20, 2014

petebacondarwin commented Jun 20, 2014

IgorMinar commented Jun 22, 2014

mgol commented Jun 22, 2014

IgorMinar commented Jun 23, 2014

mgol commented Jun 23, 2014

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

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

Conversation

lgalfaso commented Sep 13, 2013

mary-poppins commented Sep 13, 2013

petebacondarwin Sep 13, 2013

Choose a reason for hiding this comment

petebacondarwin Sep 13, 2013

Choose a reason for hiding this comment

lgalfaso Sep 13, 2013

Choose a reason for hiding this comment

mgol commented Oct 17, 2013

caitp commented Feb 5, 2014

lgalfaso commented Feb 5, 2014

caitp commented Feb 5, 2014

lgalfaso commented Feb 5, 2014

mgol commented May 12, 2014

mgol commented May 12, 2014

lgalfaso commented May 13, 2014

mgol commented May 13, 2014

petebacondarwin commented Jun 20, 2014

mgol commented Jun 20, 2014

petebacondarwin commented Jun 20, 2014

petebacondarwin commented Jun 20, 2014

mgol commented Jun 20, 2014

petebacondarwin commented Jun 20, 2014

IgorMinar commented Jun 22, 2014

mgol commented Jun 22, 2014

IgorMinar commented Jun 23, 2014

mgol commented Jun 23, 2014