fix($interpolate): use custom toString() function & fix(ngBind): use same json conversion as $interpolate #14715

Narretz · 2016-06-05T13:34:48Z

What kind of change does this PR introduce? (Bug fix, feature, docs update, ...)
bug fix

What is the current behavior? (You can also link to an open issue here)
ngBind has different binding logic for objects than interpolation

What is the new behavior (if this is a feature change)?
Same logic as interpolate

Does this PR introduce a breaking change?
Yes, the visible output is now different, i.e. consistent with interpolate. I don't think anyone relied on ngBind producing this output.
However, this PR is a step away from the proposal in #11406, which wants $interpolate to call toString() instead of toJson. See also PR #8350, and issue #7317

Please check if the PR fulfills these requirements

The commit message follows our guidelines: https://github.com/angular/angular.js/blob/master/CONTRIBUTING.md#commit-message-format
Tests for the changes have been added (for bug fixes / features)
Docs have been updated (we should probably document how this works in the guide)

Other information:

Fixes #11716

BREAKING CHANGE:

ngBind now uses the same logic as $interpolate (i.e. {{myString}}) when
binding, which means objects are now transformed with JSON.stringify. Before, it
would use the object's toString() function.

The following example shows the different output:

$scope.myObject = {a: 1, b: 2};

<span ng-bind="myObject"></span>

Before:

<span ng-bind="myObject">[object Object]</span>

After:

<span ng-bind="myObject">{"a":1,"b":2}</span>

campersau · 2016-06-05T15:25:25Z

src/ng/directive/ngBind.js

@@ -60,7 +60,7 @@ var ngBindDirective = ['$compile', function($compile) {
        $compile.$$addBindingInfo(element, attr.ngBind);
        element = element[0];
        scope.$watch(attr.ngBind, function ngBindWatchAction(value) {
-          element.textContent = isUndefined(value) ? '' : value;
+          element.textContent = isUndefined(value) ? '' : stringify(value);


This can be simplified to:

element.textContent = stringify(value);

Can you include this change too @Narretz ?

gkalpak · 2016-06-06T11:52:29Z

I think it makes sense to make ngBind and $interpolate consistent in terms of object stringification, but moving from toString to toJson will be a step backwards for objects with custom toString methods.

I think a good compromise would be to use toString if it different than Object.prototype.toString and toJson otherwise (either only on ngBind or on $interpolate too - but that's a bigger breaking change).

petebacondarwin · 2016-06-08T11:22:44Z

I agree with @gkalpak that we should use toString if a custom one is available. Given that this is a BC anyway. Let's change $interpolate to do this too and only merge to 1.6

googlebot · 2016-06-08T19:54:51Z

We found a Contributor License Agreement for you (the sender of this pull request) and all commit authors, but as best as we can tell these commits were authored by someone else. If that's the case, please add them to this pull request and have them confirm that they're okay with these commits being contributed to Google. If we're mistaken and you did author these commits, just reply here to confirm.

googlebot · 2016-06-08T19:54:51Z

We found a Contributor License Agreement for you (the sender of this pull request) and all commit authors, but as best as we can tell these commits were authored by someone else. If that's the case, please add them to this pull request and have them confirm that they're okay with these commits being contributed to Google. If we're mistaken and you did author these commits, just reply here to confirm.

Narretz · 2016-06-08T20:39:47Z

I've updated the PR to include the commit that calls custom toString() on objects. PTAL.

I've noticed a problem with our own date input tests. In the original PR, Date objects als had toString() called when interpolating them. This change made our tests fail that allow min and max attributes of date inputs to be date objects:

angular.js/test/ng/directive/inputSpec.js

Line 1896 in f3377da

    
           it('should parse ISO-based date strings as a valid max date value', function() {

. We expect a jsonified string like: '"2014-11-09T23:00:00.000Z"'. Date.toString() however produces a locale aware string that looks like this: Mon Nov 10 2014 00:00:00 GMT+0100 (Mitteleuropäische Zeit),

As a fix, I've updated the createDateParser function (

angular.js/src/ng/directive/input.js

Line 1250 in f3377da

function createDateParser(regexp, mapping) {

) to convert the passed string into an object if manual iso date parsing fails. This will produce a valid Date from the result of Date.toString(), and NaN from everything that can't be parsed (NaN is also returned if nothing works).

There's also the issue that ngMessageFormat duplicates the stringify function, and at the moment all our unit tests will use this function instead of the one defined in Angular.js. This is a bit weird, but at least it guarantees that you notice when you have made changes to the core implementation.

gkalpak · 2016-06-08T21:50:10Z

src/Angular.js

+      value = '' + value;
+      break;
+    default:
+      if (isFunction(value.toString) && value.toString !== Object.prototype.toString && !isArray(value)) {


I think we already have toString as an "IIFE global".

We do have a hasCustomToString fn

Even better 😃

gkalpak · 2016-06-09T10:55:08Z

test/ng/directive/ngBindSpec.js

+        expect(element.text()).toEqual(prop[1]);
+      });
+    });
+


I would add a test with a custom toString here too.

petebacondarwin · 2016-06-09T14:20:54Z

Perhaps we should move stringify into its own file, which is included in both angular.js and angular-message-format.js? That way we have only one version in the source code (and for testing).

petebacondarwin · 2016-06-10T11:17:05Z

Actually there already IS a stringify.js file that is shared with the angular-loader.js file. So we should make use of that, right?

Narretz · 2016-06-10T11:27:52Z

Yes, but we need to share between ng and ngMessageFormat and it looks like ngMessageFormat only needs the stringify and not the other functions

Narretz · 2016-06-22T15:33:37Z

PR has been updated, tests pass, PTAL

petebacondarwin · 2016-06-22T15:45:35Z

test/ng/directive/ngBindSpec.js

@@ -78,7 +78,7 @@ describe('ngBind*', function() {
      $rootScope.bind = new Date(2014, 10, 10, 0, 0, 0);
      element = $compile('<div ng-bind="bind"></div>')($rootScope);
      $rootScope.$digest();
-      expect(element.text()).toEqual('"2014-11-09T23:00:00.000Z"');
+      expect(element.text()).toBe('"' + $rootScope.bind.toJSON() + '"');


Unrelated to this change but, is it me, or does it seem weird to use a property called bind in this test?

petebacondarwin · 2016-06-22T15:48:57Z

The nitpick was #14715 (comment) but it looks like it is dealt with.

LGTM

Narretz · 2016-06-22T19:54:55Z

I think respecting a custom toString fn could be a breaking change for some.

I've also marked the ngBind change as breaking, but I guess this is more of a fix that now has the same consequences as using toString with interpolate.

gkalpak · 2016-06-23T18:52:51Z

docs/content/guide/interpolation.ngdoc

+If something other than a String is interpolated, the string is computed as follows:
+- If the object is not a `Number`, `Date` or `Array`, $interpolate looks for a custom `toString()`
+function on the object, and uses that. Custom means that
+`myObject.toString() !== `Object.prototype.toString()`.


This is not accurate and may confuse people. You mean myObject.toString !== Object.prototype.toString.

Thanks that's right

gkalpak · 2016-06-23T19:21:05Z

Both changes (to ngBind and $interpolate) are breaking changes imo.
Currently, the ngBind is a much bigger change, since many people might be affected by calling toJson on Date objects instead of it's toString method.

~~TBH, I think it is marginally better to call toString on Dates (instead of toJson), although that would be a significant change for $interpolate.~~

@Narretz convinced me the current behavior (i.e. calling toJson) is better 😃

Narretz · 2016-06-24T09:20:12Z

Updated again, should be good to go now

gkalpak · 2016-06-24T11:39:29Z

docs/content/guide/interpolation.ngdoc

+- `undefined` and `null` are converted to `''`
+- if the value is an object that is not a `Number`, `Date` or `Array`, $interpolate looks for
+a custom `toString()` function on the object, and uses that. Custom means that
+`myObject.toString !== `Object.prototype.toString`. If that doesn't apply, `JSON.stringify` is used.


If that doesn't apply, JSON.stringify is used.

Nitpick: I would place that on it's own bullet.

gkalpak · 2016-06-24T11:51:20Z

LGTM

BTW, in the commit message for the ngBind change, the proposed migration instructions (using a toString filter) is not 100% correct, as it will not handle undefined/null values. Besides, I believe a better/easier migration would be using value.toString() (which takes advantage of Angular's "forgiving-ness" for undefined/null expressions.

Narretz · 2016-06-24T15:08:50Z

@gkalpak Okay, so ngBind previously set undefined to empty string, and null was handled by setting textContent, which also sets it to '' (I think). So in the migration note I can special case both of them.

gkalpak · 2016-06-24T20:00:10Z

You have to special case them if you propose using a filter. But I think a filter is not necessary in this case (you can just call .toString (that's all the filter does anyway)).

@danielkrainas

Except on Numbers, Dates and Arrays. Thanks to @danielkrainas for the initial implementation of this feature. This behavior is consistent with implementations found in other languages such as Ruby, Python, and CoffeeScript. http://rubymonk.com/learning/books/1-ruby-primer/chapters/5-strings/lessons/31-string-basics https://docs.python.org/2/library/stdtypes.html#string-formatting-operations http://coffeescriptcookbook.com/chapters/strings/interpolation The commit also exposes a private $$stringify method on the angular global, so that ngMessageFormat can use the same logic without duplicating it. Fixes angular#7317 Closes angular#8350 Fixes angular#11406 BREAKING CHANGE: When converting values to strings, interpolation now uses a custom toString() function on objects that are not Number, Array or Date (custom means that the `toString` function is not the same as `Object.prototype.toString`). Otherwise, interpolation uses JSON.stringify() as usual. Should you have a custom toString() function but still want the output of JSON.stringify(), migrate as shown in the following examples: Before: ```html {{myObject}} ``` After - use the `json` filter to stringify the object: ```html {{myObject | json}} ```

Fixes angular#11716 BREAKING CHANGE: `ngBind` now uses the same logic as $interpolate (i.e. {{myString}}) when binding, which means values other than strings are now transformed as following: - null / undefined become empty string - with an object's custom toString() function, except if the object is a Date, Array, or Number - otherwise with JSON.stringify Previously, ngBind would use always use toString(). The following examples show the different output: ```js $scope.myPlainObject = {a: 1, b: 2}; $scope.myCustomObject = {a: 1, b: 2, toString: function() {return 'a+b';}}; ``` Plain Object: ```html  [object Object]  {"a":1,"b":2} ``` Object with custom toString(): ```html  [object Object]  a+b ``` If you want the output of `toString()`, you can use it directly on the value in ngBind: ```html [object Object] ```

Tests that - interpolated Date objects work for min/max - Date objects work for ng-min/ng-max

shmdhussain · 2020-02-24T13:28:27Z

angularJs Version used: 1.7.4

What is the status of this PR, I believe it is merged into master branch... but I am getting he inconsistent results when interpolating the object, sometime getting full object some time getting [object Object]

for example

{{ {id:123}  }} //sometime outputs '{id:123}'
{{ {id:123}  }} //sometime outputs [object Object]

gkalpak · 2020-02-24T13:51:07Z

This is merged and included in all version since 1.6.0-rc.0 onwards.
Even before this PR, it shouldn't cause inconsistent behavior. Your problem is caused by something else.

shmdhussain · 2020-02-24T14:01:45Z

See this screenshot, sometimes I am getting the stringified version of Object sometime getting [object Object]

Narretz added type: bug frequency: moderate needs: breaking change component: misc core severity: confusing labels Jun 5, 2016

Narretz added this to the 1.6.x milestone Jun 5, 2016

googlebot added the cla: yes label Jun 5, 2016

Narretz force-pushed the fix-ngbind-json branch from 05da211 to f3b5591 Compare June 5, 2016 14:18

campersau reviewed Jun 5, 2016
View reviewed changes

Narretz force-pushed the fix-ngbind-json branch from f3b5591 to 02971d2 Compare June 8, 2016 19:54

googlebot added cla: no and removed cla: yes labels Jun 8, 2016

Narretz changed the title ~~fix(ngBind): use same json conversion as $interpolate~~ fix(interpolate): use custom toString function & fix(ngBind): use same json conversion as $interpolate Jun 8, 2016

Narretz changed the title ~~fix(interpolate): use custom toString function & fix(ngBind): use same json conversion as $interpolate~~ fix($interpolate): use custom toString() function & fix(ngBind): use same json conversion as $interpolate Jun 8, 2016

Narretz force-pushed the fix-ngbind-json branch from 02971d2 to ef52432 Compare June 8, 2016 20:40

gkalpak reviewed Jun 8, 2016
View reviewed changes

Narretz force-pushed the fix-ngbind-json branch from d0f8273 to 9f500f3 Compare June 9, 2016 09:54

gkalpak reviewed Jun 9, 2016
View reviewed changes

Narretz force-pushed the fix-ngbind-json branch from 9f500f3 to 9d0738f Compare June 22, 2016 13:29

petebacondarwin reviewed Jun 22, 2016
View reviewed changes

petebacondarwin added the needs: merging label Jun 22, 2016

gkalpak reviewed Jun 23, 2016
View reviewed changes

Narretz force-pushed the fix-ngbind-json branch from 274b9ce to 81401f9 Compare June 24, 2016 09:05

Narretz force-pushed the fix-ngbind-json branch from 81401f9 to 5377e6a Compare June 24, 2016 10:08

gkalpak reviewed Jun 24, 2016
View reviewed changes

Narretz added 3 commits June 28, 2016 11:08

test(input): ensure Date objects work for min/max in date input types

7ce7e09

Tests that - interpolated Date objects work for min/max - Date objects work for ng-min/ng-max

Narretz force-pushed the fix-ngbind-json branch from 5377e6a to 7ce7e09 Compare June 28, 2016 09:10

Narretz merged commit 7ce7e09 into angular:master Jun 28, 2016

shmdhussain mentioned this pull request Feb 24, 2020

Interpolation should call toString on objects #11406

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix($interpolate): use custom toString() function & fix(ngBind): use same json conversion as $interpolate #14715

fix($interpolate): use custom toString() function & fix(ngBind): use same json conversion as $interpolate #14715

Narretz commented Jun 5, 2016 •

edited

Loading

campersau Jun 5, 2016

petebacondarwin Jun 22, 2016

gkalpak commented Jun 6, 2016 •

edited

Loading

petebacondarwin commented Jun 8, 2016

googlebot commented Jun 8, 2016

googlebot commented Jun 8, 2016

Narretz commented Jun 8, 2016 •

edited

Loading

gkalpak Jun 8, 2016

Narretz Jun 9, 2016

gkalpak Jun 9, 2016

gkalpak Jun 9, 2016 •

edited

Loading

petebacondarwin commented Jun 9, 2016

petebacondarwin commented Jun 10, 2016

Narretz commented Jun 10, 2016

Narretz commented Jun 22, 2016

petebacondarwin Jun 22, 2016

petebacondarwin commented Jun 22, 2016

Narretz commented Jun 22, 2016

gkalpak Jun 23, 2016

Narretz Jun 23, 2016

gkalpak commented Jun 23, 2016 •

edited

Loading

Narretz commented Jun 24, 2016

gkalpak Jun 24, 2016

gkalpak commented Jun 24, 2016

Narretz commented Jun 24, 2016

gkalpak commented Jun 24, 2016 •

edited

Loading

shmdhussain commented Feb 24, 2020 •

edited

Loading

gkalpak commented Feb 24, 2020

shmdhussain commented Feb 24, 2020

fix($interpolate): use custom toString() function & fix(ngBind): use same json conversion as $interpolate #14715

fix($interpolate): use custom toString() function & fix(ngBind): use same json conversion as $interpolate #14715

Conversation

Narretz commented Jun 5, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gkalpak commented Jun 6, 2016 • edited Loading

petebacondarwin commented Jun 8, 2016

googlebot commented Jun 8, 2016

googlebot commented Jun 8, 2016

Narretz commented Jun 8, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gkalpak Jun 9, 2016 • edited Loading

Choose a reason for hiding this comment

petebacondarwin commented Jun 9, 2016

petebacondarwin commented Jun 10, 2016

Narretz commented Jun 10, 2016

Narretz commented Jun 22, 2016

Choose a reason for hiding this comment

petebacondarwin commented Jun 22, 2016

Narretz commented Jun 22, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gkalpak commented Jun 23, 2016 • edited Loading

Narretz commented Jun 24, 2016

Choose a reason for hiding this comment

gkalpak commented Jun 24, 2016

Narretz commented Jun 24, 2016

gkalpak commented Jun 24, 2016 • edited Loading

shmdhussain commented Feb 24, 2020 • edited Loading

gkalpak commented Feb 24, 2020

shmdhussain commented Feb 24, 2020

Narretz commented Jun 5, 2016 •

edited

Loading

gkalpak commented Jun 6, 2016 •

edited

Loading

Narretz commented Jun 8, 2016 •

edited

Loading

gkalpak Jun 9, 2016 •

edited

Loading

gkalpak commented Jun 23, 2016 •

edited

Loading

gkalpak commented Jun 24, 2016 •

edited

Loading

shmdhussain commented Feb 24, 2020 •

edited

Loading