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(core): better handing of ICUs outside of i18n blocks #35347

Conversation

AndrewKushnir
Copy link
Contributor

Currently the logic that handles ICUs located outside of i18n blocks may throw exceptions at runtime. The problem is caused by the fact that we store incorrect TNode index for previous TNode (index that includes HEADER_OFFSET) and do not store a flag whether this TNode is a parent or a sibling node. As a result, the logic that assembles the final output uses incorrect TNodes and in some cases throws exceptions (when incompatible structure is extracted from tView.data due to the incorrect index). This commit adjusts the index and captures whether TNode is a parent to make sure underlying logic manipulates correct TNode.

This PR closes #35299.

PR Type

What kind of change does this PR introduce?

  • Bugfix

Does this PR introduce a breaking change?

  • Yes
  • No

@AndrewKushnir AndrewKushnir added type: bug/fix area: i18n target: patch This PR is targeted for the next patch release comp: ivy labels Feb 11, 2020
@ngbot ngbot bot added this to the needsTriage milestone Feb 11, 2020
@AndrewKushnir AndrewKushnir requested review from mhevery and removed request for petebacondarwin and pkozlowski-opensource February 11, 2020 23:16
@AndrewKushnir AndrewKushnir added the action: review The PR is still awaiting reviews from at least one requested reviewer label Feb 11, 2020
@AndrewKushnir AndrewKushnir marked this pull request as ready for review February 12, 2020 00:57
Copy link
Member

@petebacondarwin petebacondarwin left a comment

Choose a reason for hiding this comment

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

I don't really understand the ivy opcode stuff well enough to know whether this is the best/correct fix. But I see that there are decent unit tests and so happy to approve :-)

packages/core/src/render3/i18n.ts Outdated Show resolved Hide resolved
packages/core/src/render3/i18n.ts Show resolved Hide resolved
@AndrewKushnir
Copy link
Contributor Author

Thanks for review @petebacondarwin! I've applied the change that you proposed and also updated the code based on the discussion with Misko today.

@AndrewKushnir AndrewKushnir added the action: presubmit The PR is in need of a google3 presubmit label Feb 13, 2020
@AndrewKushnir AndrewKushnir force-pushed the FW-1845_icu_outside_i18n_block branch 2 times, most recently from 26be1cc to 8e78cd2 Compare February 13, 2020 22:09
Currently the logic that handles ICUs located outside of i18n blocks may throw exceptions at runtime. The problem is caused by the fact that we store incorrect TNode index for previous TNode (index that includes HEADER_OFFSET) and do not store a flag whether this TNode is a parent or a sibling node. As a result, the logic that assembles the final output uses incorrect TNodes and in some cases throws exceptions (when incompatible structure is extracted from tView.data due to the incorrect index). This commit adjusts the index and captures whether TNode is a parent to make sure underlying logic manipulates correct TNode.
@AndrewKushnir
Copy link
Contributor Author

AndrewKushnir commented Feb 13, 2020

@AndrewKushnir AndrewKushnir added action: merge The PR is ready for merge by the caretaker and removed action: presubmit The PR is in need of a google3 presubmit action: review The PR is still awaiting reviews from at least one requested reviewer labels Feb 18, 2020
@alxhub alxhub closed this in c013dd4 Feb 18, 2020
alxhub pushed a commit that referenced this pull request Feb 18, 2020
Currently the logic that handles ICUs located outside of i18n blocks may throw exceptions at runtime. The problem is caused by the fact that we store incorrect TNode index for previous TNode (index that includes HEADER_OFFSET) and do not store a flag whether this TNode is a parent or a sibling node. As a result, the logic that assembles the final output uses incorrect TNodes and in some cases throws exceptions (when incompatible structure is extracted from tView.data due to the incorrect index). This commit adjusts the index and captures whether TNode is a parent to make sure underlying logic manipulates correct TNode.

PR Close #35347
@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 20, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
action: merge The PR is ready for merge by the caretaker area: i18n cla: yes target: patch This PR is targeted for the next patch release type: bug/fix
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Angular 9: Cannot create property 'next' on number '<number>'
4 participants