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.

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

fix(ivy): reset binding index before executing a template in `refreshView` call #32201

Conversation

@AndrewKushnir
Copy link
Contributor

commented Aug 20, 2019

Prior to this change, the BINDING_INDEX of a given lView was reset after processing a template. However change detection can be triggered as a result of View queries processing, thus leading to the subsequent refreshView call (and executing a template), which in turn operated with the binding index that was not reset after the previous refreshView call. This commit updates the logic to reset binding index before we execute template, so that we always get the right binding index.

This PR resolves FW-1516.

PR Type

What kind of change does this PR introduce?

  • Bugfix

Does this PR introduce a breaking change?

  • Yes
  • No

@googlebot googlebot added the cla: yes label Aug 20, 2019

@ngbot ngbot bot modified the milestone: needsTriage Aug 20, 2019

@AndrewKushnir AndrewKushnir force-pushed the AndrewKushnir:FW-1516_binding_index_reset branch 3 times, most recently from a236fec to 9e7a773 Aug 20, 2019

@AndrewKushnir

This comment has been minimized.

Copy link
Contributor Author

commented Aug 20, 2019

@pkozlowski-opensource

This comment has been minimized.

Copy link
Member

commented Aug 20, 2019

@AndrewKushnir thnx for doing this change, I had a similar refactoring on my TODO list. Generally speaking I feel like we set lView[BINDING_INDEX] a bit "randomly" in the current code base and after your refactoring most of the other writes to lView[BINDING_INDEX] might be not necessary (especially one in the enterView).

Since you've got all the context loaded in your brain, could you see if we can remove other lView[BINDING_INDEX] assignments after your change? I can also do it in the follow-up PR.

@AndrewKushnir AndrewKushnir marked this pull request as ready for review Aug 20, 2019

@AndrewKushnir AndrewKushnir requested a review from angular/fw-core as a code owner Aug 20, 2019

@AndrewKushnir AndrewKushnir requested a review from kara Aug 20, 2019

@AndrewKushnir

This comment has been minimized.

Copy link
Contributor Author

commented Aug 20, 2019

@pkozlowski-opensource thanks for the comment!
This fix is needed to resolve some g3 targets, so I'd like to keep it as simple as possible :)
I believe you might have a better picture on this, so let's do followup changes as a separate PR.
Thank you.

@pkozlowski-opensource

This comment has been minimized.

Copy link
Member

commented Aug 20, 2019

I believe you might have a better picture on this, so let's do followup changes as a separate PR.

@AndrewKushnir sure, works for me. Basically I think that we can remove most of the assignments and just leave the one you've done. But I will do it in a separate PR as soon as yours lands.

@pkozlowski-opensource
Copy link
Member

left a comment

LGTM

@kara
kara approved these changes Aug 22, 2019
Copy link
Contributor

left a comment

LGTM

@AndrewKushnir

This comment has been minimized.

Copy link
Contributor Author

commented Aug 22, 2019

fix(ivy): reset binding index before executing a template in `refresh…
…View` call

Prior to this change, the `BINDING_INDEX` of a given lView was reset after processing a template. However change detection can be triggered as a result of View queries processing, thus leading to subsequent `refreshView` call (and executing a template), which in turn operates with the binding index that is not reset after the previous `refreshView` call. This commit updates the logic to reset binding index before we execute a template, so binding index is correct for instructions inside template function.

@AndrewKushnir AndrewKushnir force-pushed the AndrewKushnir:FW-1516_binding_index_reset branch from 9e7a773 to f8de028 Aug 22, 2019

@atscott atscott closed this in 6b245a3 Aug 22, 2019

ngdevelop-tech added a commit to ngdevelop-tech/angular that referenced this pull request Aug 27, 2019
fix(ivy): reset binding index before executing a template in `refresh…
…View` call (angular#32201)

Prior to this change, the `BINDING_INDEX` of a given lView was reset after processing a template. However change detection can be triggered as a result of View queries processing, thus leading to subsequent `refreshView` call (and executing a template), which in turn operates with the binding index that is not reset after the previous `refreshView` call. This commit updates the logic to reset binding index before we execute a template, so binding index is correct for instructions inside template function.

PR Close angular#32201
sabeersulaiman added a commit to sabeersulaiman/angular that referenced this pull request Sep 6, 2019
fix(ivy): reset binding index before executing a template in `refresh…
…View` call (angular#32201)

Prior to this change, the `BINDING_INDEX` of a given lView was reset after processing a template. However change detection can be triggered as a result of View queries processing, thus leading to subsequent `refreshView` call (and executing a template), which in turn operates with the binding index that is not reset after the previous `refreshView` call. This commit updates the logic to reset binding index before we execute a template, so binding index is correct for instructions inside template function.

PR Close angular#32201
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.