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): support for multiple ICU expressions in the same i18n block #28083

Closed
wants to merge 1 commit into from
Closed

fix(ivy): support for multiple ICU expressions in the same i18n block #28083

wants to merge 1 commit into from

Conversation

ocombe
Copy link
Contributor

@ocombe ocombe commented Jan 11, 2019

There were two issues with multiple ICU expressions in the same i18n block:

  • the regexp that was used to parse the text wasn't able to handle multiple ICU expressions, I've replaced it with parsing the text and searching for brackets (which is what we ended up doing in the end anyway)
  • we allocate node indexes for nodes generated by the ICU expressions which increases the expando value, but we would create the nodes for those cases during the update phase. In the mean time we would create some nodes during the creation phase (comment nodes for ICU expressions, text nodes, ...) with an auto increment index. This means that any node created after an ICU expression would get the following index value, but the ICU case nodes expected to use the same index as well... There was a mismatch between the auto generated index, and the expected index which was causing problems when we needed to select those nodes for updates later on. To fix it, I've added the expected node index to the list of mutate codes that we generate, and we do not use an auto increment value anymore.

FW-905 #resolve

@ocombe ocombe added type: bug/fix action: review The PR is still awaiting reviews from at least one requested reviewer area: i18n target: major This PR is targeted for the next major release comp: ivy labels Jan 11, 2019
@ocombe ocombe requested review from a team as code owners January 11, 2019 17:06
@ngbot ngbot bot modified the milestone: needsTriage Jan 11, 2019
@mary-poppins
Copy link

You can preview c53424b at https://pr28083-c53424b.ngbuilds.io/.

@ocombe ocombe force-pushed the fix/ivy/fw-905-multiple-icus branch from c53424b to 8b70117 Compare January 11, 2019 19:44
@ocombe ocombe requested a review from a team as a code owner January 11, 2019 19:44
AndrewKushnir
AndrewKushnir approved these changes Jan 11, 2019
View reviewed changes
Copy link
Contributor

@AndrewKushnir AndrewKushnir left a comment

Choose a reason for hiding this comment

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

Looks good to me.

It'd be great if @mhevery or @kara have a look as well.

Thank you.

@ocombe
fix(ivy): support for multiple ICU expressions in the same i18n block
c2d496f 
There were two issues with multiple ICU expressions in the same i18n block:
- the regexp that was used to parse the text wasn't able to handle multiple ICU expressions, I've replaced it with parsing the text and searching for brackets (which is what we ended up doing in the end anyway)
- we allocate node indexes for nodes generated by the ICU expressions which increases the expando value, but we would create the nodes for those cases during the update phase. In the mean time we would create some nodes during the creation phase (comment nodes for ICU expressions, text nodes, ...) with an auto increment index. This means that any node created after an ICU expression would get the following index value, but the ICU case nodes expected to use the same index as well... There was a mismatch between the auto generated index, and the expected index which was causing problems when we needed to select those nodes for updates later on. To fix it, I've added the expected node index to the list of mutate codes that we generate, and we do not use an auto increment value anymore.

FW-905 #resolve
@ocombe ocombe removed the request for review from a team January 13, 2019 10:10
@ocombe ocombe force-pushed the fix/ivy/fw-905-multiple-icus branch from 8b70117 to c2d496f Compare January 13, 2019 10:10
@mary-poppins
Copy link

You can preview c2d496f at https://pr28083-c2d496f.ngbuilds.io/.

@mhevery mhevery removed the action: review The PR is still awaiting reviews from at least one requested reviewer label Jan 15, 2019
@mhevery
Copy link
Contributor

mhevery commented Jan 15, 2019

http://test/OCL:229307881:BASE:229307905:1547528246221:6725d55a

@ocombe ocombe removed the request for review from kara January 15, 2019 06:13
@AndrewKushnir
Copy link
Contributor

@ocombe it looks like PR was approved and it's "green" (CirlceCI and g3 checks passed). Please set "merge" label once you're ready. Thank you.

@ocombe ocombe added the action: merge The PR is ready for merge by the caretaker label Jan 16, 2019
wKoza pushed a commit to wKoza/angular that referenced this pull request Jan 18, 2019
@ocombe @wKoza
fix(ivy): support for multiple ICU expressions in the same i18n block (…
f824e37 
…angular#28083)

There were two issues with multiple ICU expressions in the same i18n block:
- the regexp that was used to parse the text wasn't able to handle multiple ICU expressions, I've replaced it with parsing the text and searching for brackets (which is what we ended up doing in the end anyway)
- we allocate node indexes for nodes generated by the ICU expressions which increases the expando value, but we would create the nodes for those cases during the update phase. In the mean time we would create some nodes during the creation phase (comment nodes for ICU expressions, text nodes, ...) with an auto increment index. This means that any node created after an ICU expression would get the following index value, but the ICU case nodes expected to use the same index as well... There was a mismatch between the auto generated index, and the expected index which was causing problems when we needed to select those nodes for updates later on. To fix it, I've added the expected node index to the list of mutate codes that we generate, and we do not use an auto increment value anymore.

FW-905 #resolve
PR Close angular#28083
wKoza pushed a commit to wKoza/angular that referenced this pull request Jan 18, 2019
@ocombe @wKoza
fix(ivy): support for multiple ICU expressions in the same i18n block (…
25d7435 
…angular#28083)

There were two issues with multiple ICU expressions in the same i18n block:
- the regexp that was used to parse the text wasn't able to handle multiple ICU expressions, I've replaced it with parsing the text and searching for brackets (which is what we ended up doing in the end anyway)
- we allocate node indexes for nodes generated by the ICU expressions which increases the expando value, but we would create the nodes for those cases during the update phase. In the mean time we would create some nodes during the creation phase (comment nodes for ICU expressions, text nodes, ...) with an auto increment index. This means that any node created after an ICU expression would get the following index value, but the ICU case nodes expected to use the same index as well... There was a mismatch between the auto generated index, and the expected index which was causing problems when we needed to select those nodes for updates later on. To fix it, I've added the expected node index to the list of mutate codes that we generate, and we do not use an auto increment value anymore.

FW-905 #resolve
PR Close angular#28083
ngfelixl pushed a commit to ngfelixl/angular that referenced this pull request Jan 28, 2019
@ocombe @ngfelixl
fix(ivy): support for multiple ICU expressions in the same i18n block (…
53fdd80 
…angular#28083)

There were two issues with multiple ICU expressions in the same i18n block:
- the regexp that was used to parse the text wasn't able to handle multiple ICU expressions, I've replaced it with parsing the text and searching for brackets (which is what we ended up doing in the end anyway)
- we allocate node indexes for nodes generated by the ICU expressions which increases the expando value, but we would create the nodes for those cases during the update phase. In the mean time we would create some nodes during the creation phase (comment nodes for ICU expressions, text nodes, ...) with an auto increment index. This means that any node created after an ICU expression would get the following index value, but the ICU case nodes expected to use the same index as well... There was a mismatch between the auto generated index, and the expected index which was causing problems when we needed to select those nodes for updates later on. To fix it, I've added the expected node index to the list of mutate codes that we generate, and we do not use an auto increment value anymore.

FW-905 #resolve
PR Close angular#28083
@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 Sep 14, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@mhevery mhevery mhevery approved these changes

@AndrewKushnir AndrewKushnir AndrewKushnir approved these changes

Assignees
No one assigned
Labels
action: merge The PR is ready for merge by the caretaker area: i18n cla: yes target: major This PR is targeted for the next major release type: bug/fix
Projects
None yet
Milestone
needsTriage
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@ocombe @mary-poppins @mhevery @AndrewKushnir @googlebot