feat(input): constant exprs for ngTrueValue/ngFalseValue #5346

caitp · 2013-12-10T03:05:05Z

If ngTrueValue / ngFalseValue are strings evaluating to constant expressions,
the constant value is used in place of the string value.

If the value is not a constant, its string value is used, as they were
previously.

If no string expression is provided, then the default value remains true or
false.

Fixes #1199

mary-poppins · 2013-12-10T03:05:08Z

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!

IgorMinar · 2013-12-10T03:12:55Z

I'm sorry, but I wasn't able to verify your 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 me 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.

IgorMinar · 2013-12-10T23:12:15Z

CLA signature verified! Thank you!

Someone from the team will now triage your PR and it will be processed based on the determined priority (doc updates and fixes with tests are prioritized over other changes).

IgorMinar · 2013-12-21T01:33:16Z

src/ng/directive/input.js

@@ -810,14 +819,14 @@ function checkboxInputType(scope, element, attr, ctrl) {
      </doc:scenario>
    </doc:example>
 */
-var inputDirective = ['$browser', '$sniffer', function($browser, $sniffer) {
+var inputDirective = ['$browser', '$sniffer', '$injector', function($browser, $sniffer, $injector) {


why not inject $parse directly? I think that would be better. try to avoid direct interactions with injector whenever possible. otherwise the dependencies become hard to follow and you end up using service location rather than dependency injection.

My thinking was, different input types might need to inject different things ($interpolate / $parse / $whatever) in the future... But if it's okay to pile on extra things, then there's probably no harm in just injecting it directly.

IgorMinar · 2013-12-21T01:35:37Z

This looks good to me, except for one thing - have you thought about what should happen when someone points ng-true-value to a path like user.name and that path evaluates to something?

caitp · 2013-12-21T01:42:55Z

I guess my original thinking was, constants would be good because you wouldn't expect them to be re-evaluated each time, but I suppose there's no reason it would have to be constant

caitp · 2013-12-21T01:54:59Z

Changed it to not throw away non-constant expressions, it throws away undefined when the expression is not "undefined" now... This is really all to avoid a breaking change, it could be updated to just use the parsed value regardless though.

IgorMinar · 2013-12-21T02:22:50Z

test/ng/directive/inputSpec.js

+      expect(inputElm[0].checked).toBe(false);
+
+      browserTrigger(inputElm, 'click');
+      expect(scope.value.num).toEqual(12345);


IgorMinar · 2013-12-21T02:28:26Z

you are missing a test for the user.name case, which brings up a new issue - what if the value of that path changes at runtime?

If ngTrueValue / ngFalseValue are strings, they are evaluated as expressions. If the resulting value is undefined, and not the "undefined" constant, then it is treated as a string literal. Otherwise, the evaluated value is used. If no string expression is provided, then the default value remains true or false.

caitp · 2013-12-21T02:46:34Z

So I guess, regarding the other points you've mentioned, it comes down to:

a) should getter be re-evaluated each time the formatter is run

b) should deep equality check be used rather than ===

for a), checking only once is closer to the current implementation (but maybe that's not desirable), for b) if it's a really complicated object (which is probably an unlikely scenario for a checkbox), it will hurt --- but in most cases it should be short circuited by the identity equality check in angular.equals(). I am not sure how frequently ngTrueValue and ngFalseValue actually get used, so it may not matter a whole lot that in some cases there can be a performance hit.

To avoid the function call, an external === check could happen first, similar to in $watch, but I'm not sure there would be any meaningful performance benefit there. Anyways, if we're going to make this a breaking change, then we probably don't want to merge it for next week, and have some time to come up with a decision about those things, yeah?

caitp · 2014-01-07T01:06:37Z

This could also be related to #4970

chrisirhc · 2014-01-07T04:00:22Z

How about adding an Exp suffix (e.g. ngTrueExp or ngTrueValueExp) for attributes that expect an expression? This way, you can avoid making a breaking change and slowing down existing implementations.

It might look a little ugly, but I'm thinking some sort of convention across directives for expressions that are "live" would make sense (e.g. ngMaxLengthExp).

caitp · 2014-01-07T04:07:41Z

@IgorMinar what do you think about @chrisirhc's suggestion? Since cdc4d48 landed, it might not cause code to inflate TOO much to have two implementations. But maybe two is too many

btford · 2014-06-27T18:28:05Z

Ping

IgorMinar · 2014-06-28T14:23:20Z

We should not add another set of attributes just to deal with expressions. I'd rather break this api to fix it since it's not a frequently used one.

How about a compromise where we check if the expression is constant and if it is we'll go ahead with otherwise we throw an error saying that non-constant expressions are not currently supported?

Alternatively we could support non-constants but we'd have to set up watches for ngTrueValue and ngFalseValue and update the checkbox state should the watch fire. This would mean that the checkbox would be controlled by 3 watches: ngModel, ngTrueValue and ngFalseValue. Which might cause some really odd behavior.

IgorMinar · 2014-07-01T00:10:10Z

@caitp?

what do you think about the changes I proposed? supporting non-constants should be trivial, supporting all expressions is more tricky but you can give it a shot (we'll need tests for the scenarios where true/false expression changes at runtime).

I'm thinking that the trueValue comparison should always take a precedence over false value so if both true value and false value are the same, we should consider the checkbox to be checked.

caitp · 2014-07-01T00:28:16Z

what do you think about the changes I proposed? supporting non-constants should be trivial, supporting all expressions is more tricky but you can give it a shot (we'll need tests for the scenarios where true/false expression changes at runtime).

@IgorMinar so you're saying for 1.3 we should just break this and use parsed expressions? I can fix this up to do that and add some better tests

I'm thinking that the trueValue comparison should always take a precedence over false value so if both true value and false value are the same, we should consider the checkbox to be checked.

I think if trueValue===falseValue we should probably emit some kind of warning, because that doesn't seem like a sensible thing to do. Maybe a warning that could be compiled out in release builds if we ever get that working

IgorMinar · 2014-07-01T18:43:56Z

I can't see how we could preserve backwards compatibility here without introducing new attributes or magic that is going to burn people, both are only more confusing.

wrt warning: we don't emit a warning today and you can set the trueValue and falseValue to the same string.

I keep on going back and forth between supporting constants only vs supporting all expressions. What's your take on that? My thinking is that supporting constants would deal with 90% of use cases without the complexity associated with supporting full expressions.

caitp · 2014-07-01T20:49:48Z

I keep on going back and forth between supporting constants only vs supporting all expressions. What's your take on that? My thinking is that supporting constants would deal with 90% of use cases without the complexity associated with supporting full expressions.

Supporting constants only, you gain the benefit of not having to watch the expression, but maybe bind-once is a better solution for that in 1.3, so maybe adding another $watch isn't so bad here.

IgorMinar · 2014-07-01T20:54:34Z

let's do constants only and throw an error for non-constants.

one time binding is not going to help much because it's not so much the
cost of watching but complexity related resolving state mutations that is
troublesome.

On Tue, Jul 1, 2014 at 1:49 PM, Caitlin Potter notifications@github.com
wrote:

I keep on going back and forth between supporting constants only vs
supporting all expressions. What's your take on that? My thinking is that
supporting constants would deal with 90% of use cases without the
complexity associated with supporting full expressions.

Supporting constants only, you gain the benefit of not having to watch the
expression, but maybe bind-once is a better solution for that in 1.3, so
maybe adding another $watch isn't so bad here.

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

ngTrueValue and ngFalseValue now support parsed expressions which the parser determines to be constant values. BREAKING CHANGE: Previously, these attributes would always be treated as strings. However, they are now parsed as expressions, and will throw if an expression is non-constant. To convert non-constant strings into constant expressions, simply wrap them in an extra pair of quotes, like so: <input type="checkbox" ng-model="..." ng-true-value="'truthyValue'"> Closes angular#8041 Closes angular#5346 Closes angular#1199

ZAYEC77 · 2014-10-13T11:59:17Z

Let's add two another directives, e.g.: ngTrueExpr and ngFalseExpr.
What about it?

btford · 2014-10-13T17:00:43Z

@ZAYEC77 – I think this is an interesting idea, but I don't think it belongs in core. Still, if you want this feature, you can easily implement and publish it as a 3rd party module with npm or bower.

caitp mentioned this pull request Dec 10, 2013

NgMinlength And NgMaxlength - Set length value dynamically not working #5319

Closed

caitp mentioned this pull request Dec 19, 2013

input [checkbox] ng-true-value="1" required - never validates #4681

Closed

ghost assigned IgorMinar Dec 21, 2013

IgorMinar reviewed Dec 21, 2013
View reviewed changes

caitp mentioned this pull request Jul 2, 2014

feat(input): support constant expressions for ngTrueValue/ngFalseValue #8041

Closed

caitp closed this in c90cefe Jul 2, 2014

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(input): constant exprs for ngTrueValue/ngFalseValue #5346

feat(input): constant exprs for ngTrueValue/ngFalseValue #5346

caitp commented Dec 10, 2013

mary-poppins commented Dec 10, 2013

IgorMinar commented Dec 10, 2013

IgorMinar commented Dec 10, 2013

IgorMinar Dec 21, 2013

caitp Dec 21, 2013

IgorMinar commented Dec 21, 2013

caitp commented Dec 21, 2013

caitp commented Dec 21, 2013

IgorMinar Dec 21, 2013

IgorMinar commented Dec 21, 2013

caitp commented Dec 21, 2013

caitp commented Jan 7, 2014

chrisirhc commented Jan 7, 2014

caitp commented Jan 7, 2014

btford commented Jun 27, 2014

IgorMinar commented Jun 28, 2014

IgorMinar commented Jul 1, 2014

caitp commented Jul 1, 2014

IgorMinar commented Jul 1, 2014

caitp commented Jul 1, 2014

IgorMinar commented Jul 1, 2014

ZAYEC77 commented Oct 13, 2014

btford commented Oct 13, 2014

feat(input): constant exprs for ngTrueValue/ngFalseValue #5346

feat(input): constant exprs for ngTrueValue/ngFalseValue #5346

Conversation

caitp commented Dec 10, 2013

mary-poppins commented Dec 10, 2013

IgorMinar commented Dec 10, 2013

IgorMinar commented Dec 10, 2013

IgorMinar Dec 21, 2013

Choose a reason for hiding this comment

caitp Dec 21, 2013

Choose a reason for hiding this comment

IgorMinar commented Dec 21, 2013

caitp commented Dec 21, 2013

caitp commented Dec 21, 2013

IgorMinar Dec 21, 2013

Choose a reason for hiding this comment

IgorMinar commented Dec 21, 2013

caitp commented Dec 21, 2013

caitp commented Jan 7, 2014

chrisirhc commented Jan 7, 2014

caitp commented Jan 7, 2014

btford commented Jun 27, 2014

IgorMinar commented Jun 28, 2014

IgorMinar commented Jul 1, 2014

caitp commented Jul 1, 2014

IgorMinar commented Jul 1, 2014

caitp commented Jul 1, 2014

IgorMinar commented Jul 1, 2014

ZAYEC77 commented Oct 13, 2014

btford commented Oct 13, 2014