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

fix(ivy): host-styling throws assert exception inside *ngFor #35133

Closed
wants to merge 2 commits into from
Closed

fix(ivy): host-styling throws assert exception inside *ngFor #35133

wants to merge 2 commits into from

Conversation

mhevery
Copy link
Contributor

@mhevery mhevery commented Feb 3, 2020

Inside *ngFor the second run of the styling instructions can get into situation where it tries to read a value from a binding which has not yet executed. As a result the read is NO_CHANGE value and subsequent property read cause an exception as it is of wrong type.

Fix #35118

PR Checklist

Please check if your PR fulfills the following requirements:

PR Type

What kind of change does this PR introduce?

  • Bugfix
  • Feature
  • Code style update (formatting, local variables)
  • Refactoring (no functional changes, no api changes)
  • Build related changes
  • CI related changes
  • Documentation content changes
  • angular.io application / infrastructure changes
  • Other... Please describe:

What is the current behavior?

Issue Number: N/A

What is the new behavior?

Does this PR introduce a breaking change?

  • Yes
  • No

Other information

@mhevery
fix(ivy): host-styling throws assert exception inside *ngFor
76571d7 
Inside `*ngFor` the second run of the styling instructions can get into situation where it tries to read a value from a binding which has not yet executed. As a result the read is `NO_CHANGE` value and subsequent property read cause an exception as it is of wrong type.

Fix angular#35118
@mhevery mhevery requested a review from kara February 3, 2020 20:33
@mhevery mhevery added area: core Issues related to the framework runtime action: merge The PR is ready for merge by the caretaker target: patch This PR is targeted for the next patch release labels Feb 3, 2020
@ngbot ngbot bot modified the milestone: needsTriage Feb 3, 2020
@mhevery
Copy link
Contributor Author

mhevery commented Feb 3, 2020 •
edited

✅ presubmit

kara
kara approved these changes Feb 3, 2020
View reviewed changes
Copy link
Contributor

@kara kara left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, with some nits

packages/core/src/render3/instructions/styling.ts Outdated Show resolved Hide resolved
packages/core/test/acceptance/styling_spec.ts Outdated Show resolved Hide resolved
packages/core/test/acceptance/styling_spec.ts Outdated Show resolved Hide resolved
packages/core/test/acceptance/styling_spec.ts Outdated Show resolved Hide resolved
packages/core/test/acceptance/styling_spec.ts Show resolved Hide resolved
packages/core/src/render3/instructions/styling.ts Show resolved Hide resolved
@mhevery mhevery closed this in ab931cf Feb 3, 2020
mhevery added a commit that referenced this pull request Feb 3, 2020
@mhevery
fix(ivy): host-styling throws assert exception inside *ngFor (#35133)
31e9873 
Inside `*ngFor` the second run of the styling instructions can get into situation where it tries to read a value from a binding which has not yet executed. As a result the read is `NO_CHANGE` value and subsequent property read cause an exception as it is of wrong type.

Fix #35118

PR Close #35133
diicar
diicar reviewed Feb 4, 2020
View reviewed changes
packages/core/test/bundling/todo/bundle.golden_symbols.json
@@ -47,6 +47,9 @@
{
"name": "EMPTY_ARRAY"
},
{
"name": "EMPTY_ARRAY"
},
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

EMPTY_ARRAY already exists

sonukapoor pushed a commit to sonukapoor/angular that referenced this pull request Feb 13, 2020
@mhevery @sonukapoor
fix(ivy): host-styling throws assert exception inside *ngFor (angular…
97c5f77 
…#35133)

Inside `*ngFor` the second run of the styling instructions can get into situation where it tries to read a value from a binding which has not yet executed. As a result the read is `NO_CHANGE` value and subsequent property read cause an exception as it is of wrong type.

Fix angular#35118

PR Close angular#35133
sonukapoor pushed a commit to sonukapoor/angular that referenced this pull request Feb 17, 2020
@mhevery @sonukapoor
fix(ivy): host-styling throws assert exception inside *ngFor (angular…
dbb2d63 
…#35133)

Inside `*ngFor` the second run of the styling instructions can get into situation where it tries to read a value from a binding which has not yet executed. As a result the read is `NO_CHANGE` value and subsequent property read cause an exception as it is of wrong type.

Fix angular#35118

PR Close angular#35133
@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 Mar 6, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@kara kara kara approved these changes

@diicar diicar diicar left review comments

Assignees
No one assigned
Labels
action: merge The PR is ready for merge by the caretaker area: core Issues related to the framework runtime cla: yes target: patch This PR is targeted for the next patch release
Projects
None yet
Milestone
needsTriage
Development

Successfully merging this pull request may close these issues.

Since RC.11 Class HostBinding throws an error
4 participants
@mhevery @kara @diicar @googlebot