Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Angular Ivy causes objects not rendering correctly when implemented toString and valueOf #38839

Closed
jmwierzbicki opened this issue Sep 14, 2020 · 4 comments
Closed

Angular Ivy causes objects not rendering correctly when implemented toString and valueOf #38839

jmwierzbicki opened this issue Sep 14, 2020 · 4 comments
Assignees
@crisbeto
Labels
area: core Issues related to the framework runtime core: basic template syntax needs: discussion Indicates than an issue is an open discussion P2 The issue is important to a large percentage of users, with a workaround regression Indicates than the issue relates to something that worked in a previous version state: has PR
Milestone
Backlog

Comments

@jmwierzbicki
Copy link

jmwierzbicki commented Sep 14, 2020 •
edited

🐞 bug report

Affected Package

The issue is caused by package @angular/compiler

Is this a regression?

Yes, the previous version in which this bug was not present was: any version with IVY disabled

Description

When IVY compiler is enabled, every object that is passed in interpollation marks tries to use .valueOf() method before .toString(), because of that it is not possible for creation of flexible objects that can take form of numbers if needed.

In previous version it was possible to create such constructions:

{{ testOb }} // renders 'some string'
{{ testOb + ' foo' }} //renders '0 foo'
{{ testOb.valueOf()}} // renders 0
{{ testOb++}} // renders 1 without ivy and crashes at ivy

Also when passing Proxy object to template, console error is thrown. like it can not find toString method at all.

🔬 Minimal Reproduction

examples with IVY enabled and disabled:

https://stackblitz.com/edit/angular-ivy-af1zfj?file=src%2Fapp%2Fapp.component.ts
https://stackblitz.com/edit/angular-wwwg7j?file=src%2Fapp%2Fapp.component.html

🌍 Your Environment

Angular Version:




@angular-devkit/architect         0.1000.5
@angular-devkit/build-angular     0.1000.5
@angular-devkit/build-optimizer   0.1000.5
@angular-devkit/build-webpack     0.1000.5
@angular-devkit/core              10.0.5
@angular-devkit/schematics        10.0.5
@angular/cdk                      8.2.3
@angular/cli                      10.0.5
@angular/material                 8.2.3
@ngtools/webpack                  10.0.5
@schematics/angular               10.0.5
@schematics/update                0.1000.5
rxjs                              6.5.4
typescript                        3.9.7
webpack                           4.43.0
@AndrewKushnir AndrewKushnir added the area: core Issues related to the framework runtime label Sep 14, 2020
@ngbot ngbot bot added this to the needsTriage milestone Sep 14, 2020
@pmartijena
Copy link
Contributor

As a temporary fix, couldn't you cast it to a number? E.g. Number(testOb)++

@jmwierzbicki
Copy link
Author

jmwierzbicki commented Sep 22, 2020 •
edited

not really, we got special case in our project, that some properties in objects pulled from api might be encrypted - this is also true for whole lists. Our goal is to display data with placeholders, without breaking layouts. Solution we came with was Proxied object. With these we could pass this special object inside *ngFor, and every reference to value or string was filled with placeholders, while subproperties was generated on fly.

it worked flawlessy until we enabled angular ivy

The idea was not to touch any templates, because we got over thousand of components by now, and handling this case everywhere would be catastrophic

@ngbot ngbot bot modified the milestones: needsTriage, Backlog Sep 22, 2020
@petebacondarwin petebacondarwin added the regression Indicates than the issue relates to something that worked in a previous version label Sep 22, 2020
@crisbeto
Copy link
Member

crisbeto commented Sep 28, 2020 •
edited

The reason it works this way is because we convert values to strings using '' + value (see https://github.com/angular/angular/blob/master/packages/core/src/render3/util/misc_utils.ts#L20). We can make it work with something like String(value), but we have to be very careful when changing the renderStringify function, because every single binding in the app goes through it after every change.

@jelbourn jelbourn added P2 The issue is important to a large percentage of users, with a workaround needs: discussion Indicates than an issue is an open discussion and removed type: use-case labels Oct 1, 2020
@crisbeto crisbeto self-assigned this Nov 25, 2020
crisbeto added a commit to crisbeto/angular that referenced this issue Nov 25, 2020
@crisbeto
fix(core): not invoking object's toString when rendering to the DOM
8f08e52 
Currently we convert objects to strings using `'' + value` which is quickest,
but it stringifies the value using its `valueOf`, rather than `toString`. These
changes switch to using `String(value)` which has identical performance
and calls the `toString` method as expected. Note that another option
was calling `toString` directly, but benchmarking showed it to be slower.

I've included the benchmark I used to verify the performance so we have it
for future reference and we can reuse it when making changes to `renderStringify`
in the future.

Also for reference, here are the results of the benchmark:

```
Benchmark: renderStringify
 concat: 2.006 ns(0%)
 concat with toString: 2.201 ns(-10%)
 toString: 237.494 ns(-11741%)
 toString with toString: 121.072 ns(-5937%)
 constructor: 2.201 ns(-10%)
 constructor with toString: 2.201 ns(-10%)
 toString mono: 14.536 ns(-625%)
 toString with toString mono: 9.757 ns(-386%)
```

Fixes angular#38839.
@crisbeto crisbeto mentioned this issue Nov 25, 2020
Closed
crisbeto added a commit to crisbeto/angular that referenced this issue Nov 25, 2020
@crisbeto
fix(core): not invoking object's toString when rendering to the DOM
fbebcd5 
Currently we convert objects to strings using `'' + value` which is quickest,
but it stringifies the value using its `valueOf`, rather than `toString`. These
changes switch to using `String(value)` which has identical performance
and calls the `toString` method as expected. Note that another option
was calling `toString` directly, but benchmarking showed it to be slower.

I've included the benchmark I used to verify the performance so we have it
for future reference and we can reuse it when making changes to `renderStringify`
in the future.

Also for reference, here are the results of the benchmark:

```
Benchmark: renderStringify
 concat: 2.006 ns(0%)
 concat with toString: 2.201 ns(-10%)
 toString: 237.494 ns(-11741%)
 toString with toString: 121.072 ns(-5937%)
 constructor: 2.201 ns(-10%)
 constructor with toString: 2.201 ns(-10%)
 toString mono: 14.536 ns(-625%)
 toString with toString mono: 9.757 ns(-386%)
```

Fixes angular#38839.
crisbeto added a commit to crisbeto/angular that referenced this issue Nov 25, 2020
@crisbeto
fix(core): not invoking object's toString when rendering to the DOM
9488506 
Currently we convert objects to strings using `'' + value` which is quickest,
but it stringifies the value using its `valueOf`, rather than `toString`. These
changes switch to using `String(value)` which has identical performance
and calls the `toString` method as expected. Note that another option
was calling `toString` directly, but benchmarking showed it to be slower.

I've included the benchmark I used to verify the performance so we have it
for future reference and we can reuse it when making changes to `renderStringify`
in the future.

Also for reference, here are the results of the benchmark:

```
Benchmark: renderStringify
 concat: 2.006 ns(0%)
 concat with toString: 2.201 ns(-10%)
 toString: 237.494 ns(-11741%)
 toString with toString: 121.072 ns(-5937%)
 constructor: 2.201 ns(-10%)
 constructor with toString: 2.201 ns(-10%)
 toString mono: 14.536 ns(-625%)
 toString with toString mono: 9.757 ns(-386%)
```

Fixes angular#38839.
crisbeto added a commit to crisbeto/angular that referenced this issue Nov 28, 2020
@crisbeto
fix(core): not invoking object's toString when rendering to the DOM
29f855d 
Currently we convert objects to strings using `'' + value` which is quickest,
but it stringifies the value using its `valueOf`, rather than `toString`. These
changes switch to using `String(value)` which has identical performance
and calls the `toString` method as expected. Note that another option
was calling `toString` directly, but benchmarking showed it to be slower.

I've included the benchmark I used to verify the performance so we have it
for future reference and we can reuse it when making changes to `renderStringify`
in the future.

Also for reference, here are the results of the benchmark:

```
Benchmark: renderStringify
 concat: 2.006 ns(0%)
 concat with toString: 2.201 ns(-10%)
 toString: 237.494 ns(-11741%)
 toString with toString: 121.072 ns(-5937%)
 constructor: 2.201 ns(-10%)
 constructor with toString: 2.201 ns(-10%)
 toString mono: 14.536 ns(-625%)
 toString with toString mono: 9.757 ns(-386%)
```

Fixes angular#38839.
crisbeto added a commit to crisbeto/angular that referenced this issue Nov 28, 2020
@crisbeto
fix(core): not invoking object's toString when rendering to the DOM
4893f2b 
Currently we convert objects to strings using `'' + value` which is quickest,
but it stringifies the value using its `valueOf`, rather than `toString`. These
changes switch to using `String(value)` which has identical performance
and calls the `toString` method as expected. Note that another option
was calling `toString` directly, but benchmarking showed it to be slower.

I've included the benchmark I used to verify the performance so we have it
for future reference and we can reuse it when making changes to `renderStringify`
in the future.

Also for reference, here are the results of the benchmark:

```
Benchmark: renderStringify
 concat: 2.006 ns(0%)
 concat with toString: 2.201 ns(-10%)
 toString: 237.494 ns(-11741%)
 toString with toString: 121.072 ns(-5937%)
 constructor: 2.201 ns(-10%)
 constructor with toString: 2.201 ns(-10%)
 toString mono: 14.536 ns(-625%)
 toString with toString mono: 9.757 ns(-386%)
```

Fixes angular#38839.
jessicajaniuk pushed a commit that referenced this issue Nov 30, 2020
@crisbeto @thePunderWoman
fix(core): not invoking object's toString when rendering to the DOM (#…
75e22ab 
…39843)

Currently we convert objects to strings using `'' + value` which is quickest,
but it stringifies the value using its `valueOf`, rather than `toString`. These
changes switch to using `String(value)` which has identical performance
and calls the `toString` method as expected. Note that another option
was calling `toString` directly, but benchmarking showed it to be slower.

I've included the benchmark I used to verify the performance so we have it
for future reference and we can reuse it when making changes to `renderStringify`
in the future.

Also for reference, here are the results of the benchmark:

```
Benchmark: renderStringify
 concat: 2.006 ns(0%)
 concat with toString: 2.201 ns(-10%)
 toString: 237.494 ns(-11741%)
 toString with toString: 121.072 ns(-5937%)
 constructor: 2.201 ns(-10%)
 constructor with toString: 2.201 ns(-10%)
 toString mono: 14.536 ns(-625%)
 toString with toString mono: 9.757 ns(-386%)
```

Fixes #38839.

PR Close #39843
@angular-automatic-lock-bot
Copy link

This issue has been automatically locked due to inactivity.
Please file a new issue if you are encountering a similar or related problem.

Read more about our automatic conversation locking policy.

This action has been performed automatically by a bot.

@angular-automatic-lock-bot angular-automatic-lock-bot bot locked and limited conversation to collaborators Dec 31, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees

@crisbeto crisbeto

Labels
area: core Issues related to the framework runtime core: basic template syntax needs: discussion Indicates than an issue is an open discussion P2 The issue is important to a large percentage of users, with a workaround regression Indicates than the issue relates to something that worked in a previous version state: has PR
Projects
None yet
Milestone
Backlog
Development

Successfully merging a pull request may close this issue.

fix(core): not invoking object's toString when rendering to the DOM crisbeto/angular
7 participants
@petebacondarwin @sonukapoor @jelbourn @crisbeto @pmartijena @jmwierzbicki @AndrewKushnir