The Wayback Machine - https://web.archive.org/web/20211019060940/https://github.com/angular/angular/pull/39233
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

Jump to bottom

i18n cleanup 5 #39233

Closed
wants to merge 12 commits into from
Closed

i18n cleanup 5 #39233

wants to merge 12 commits into from

Conversation

@mhevery
Copy link
Contributor

@mhevery mhevery commented Oct 13, 2020 •
edited

This PR builds upon #39003. Only review the last refactor(core): changes.

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
@google-cla google-cla bot added the cla: yes label Oct 13, 2020
@AndrewKushnir AndrewKushnir self-requested a review Oct 13, 2020
@mhevery mhevery force-pushed the i18n_cleanup_5 branch from 0d02f0f to 6ce163c Oct 13, 2020
@ngbot ngbot bot added this to the needsTriage milestone Oct 13, 2020
@mhevery mhevery force-pushed the i18n_cleanup_5 branch 2 times, most recently from ceb2013 to 8bea557 Oct 14, 2020
AndrewKushnir
AndrewKushnir reviewed Oct 14, 2020
View changes
packages/core/src/render3/interfaces/node.ts Outdated Show resolved Hide resolved
packages/core/src/render3/component.ts Show resolved Hide resolved
packages/core/src/render3/component.ts Outdated Show resolved Hide resolved
packages/core/src/render3/instructions/shared.ts Outdated Show resolved Hide resolved
packages/core/src/render3/instructions/shared.ts Outdated Show resolved Hide resolved
packages/core/src/render3/instructions/shared.ts Outdated Show resolved Hide resolved
packages/core/src/render3/util/view_utils.ts Show resolved Hide resolved
packages/core/src/render3/view_engine_compatibility.ts Show resolved Hide resolved
AndrewKushnir
AndrewKushnir reviewed Oct 14, 2020
View changes
packages/core/src/render3/di.ts Outdated Show resolved Hide resolved
packages/core/src/render3/i18n/i18n_parse.ts Outdated Show resolved Hide resolved
packages/core/src/render3/instructions/i18n.ts Outdated Show resolved Hide resolved
packages/core/src/render3/instructions/shared.ts Show resolved Hide resolved
packages/core/src/render3/instructions/template.ts Outdated Show resolved Hide resolved
packages/core/src/render3/i18n/i18n_parse.ts Outdated Show resolved Hide resolved
packages/core/src/render3/instructions/i18n_icu_container_visitor.ts Outdated Show resolved Hide resolved
packages/core/src/render3/instructions/i18n_icu_container_visitor.ts Show resolved Hide resolved
packages/core/src/render3/i18n/i18n_debug.ts Show resolved Hide resolved
packages/core/src/render3/i18n/i18n_parse.ts Show resolved Hide resolved
petebacondarwin
petebacondarwin reviewed Oct 14, 2020
View changes
Copy link
Member

@petebacondarwin petebacondarwin left a comment

Typo in refactor(core): Use ~x instead of -x which can result in -0

paste -> past

@pullapprove pullapprove bot requested a review from IgorMinar Oct 14, 2020
@mhevery mhevery force-pushed the i18n_cleanup_5 branch 2 times, most recently from 0824016 to 70a15ef Oct 15, 2020
AndrewKushnir
AndrewKushnir approved these changes Oct 15, 2020
View changes
Copy link
Contributor

@AndrewKushnir AndrewKushnir left a comment

LGTM, thanks Misko! 👍 (just one minor comment)

packages/core/src/render3/component.ts Show resolved Hide resolved
@mhevery mhevery force-pushed the i18n_cleanup_5 branch from 70a15ef to 7abd7db Oct 15, 2020
@mhevery mhevery mentioned this pull request Oct 16, 2020
Closed
14 tasks
@mhevery mhevery force-pushed the i18n_cleanup_5 branch from 7abd7db to 19b3ff8 Oct 16, 2020
@AndrewKushnir AndrewKushnir removed the request for review from IgorMinar Oct 22, 2020
@mhevery
refactor(core): clean up circular dependencies
b4a077c 
Moved code from `interfaces/i18n.ts` which was causing circular dependencies
@mhevery mhevery force-pushed the i18n_cleanup_5 branch from 07b3ab3 to b4a077c Oct 22, 2020
alxhub added a commit that referenced this issue Oct 22, 2020
@mhevery @alxhub
refactor(core): Change TemplateFixture to named parameters (#39233)
f875b0d 
`TemplateFixture` used to have positional parameters and many tests got
hard to read as number of parameters reach 10+ with many of them `null`.
This refactoring changes `TemplateFixture` to take named parameters
which improves usability and readability in tests.

PR Close #39233
alxhub added a commit that referenced this issue Oct 22, 2020
@mhevery @alxhub
refactor(core): move i18n_spec.ts into i18n subfolder (#39233)
c83a389 
`i18n_spec.ts` file was incorrectly in the `render3` folder rather than `render3/i18n`

PR Close #39233
alxhub added a commit that referenced this issue Oct 22, 2020
@mhevery @alxhub
fix(core): Store ICU state in LView rather than in TView (#39233)
0992b67 
Before this refactoring/fix the ICU would store the current selected
index in `TView`. This is incorrect, since if ICU is in `ngFor` it will
cause issues in some circumstances. This refactoring properly moves the
state to `LView`.

closes #37021
closes #38144
closes #38073

PR Close #39233
alxhub added a commit that referenced this issue Oct 22, 2020
@mhevery @alxhub
refactor(core): Change TName.tagName to a more generic value name. (
4e29200 
#39233)

This is a pre-requisite for making the `TNode.value` a generic storage
mechanism for attaching data to `TNode`.

PR Close #39233
alxhub added a commit that referenced this issue Oct 22, 2020
@mhevery @alxhub
refactor(core): Remove hack where we TIcu was stored in tagName (#…
7d43ca9 
…39233)

Remove casting where we stored `TIcu` in `TNode.tagName` which was of
type `string` rather than `TIcu'. (renamed to `TNode.value` in previous
commit.)

PR Close #39233
alxhub added a commit that referenced this issue Oct 22, 2020
@mhevery @alxhub
refactor(core): Create TNodeType.Text to display full template in `…
279de9f 
…TView` debug (#39233)

When looking at `TView` debug template only Element nodes were displayed
as `TNode.Element` was used for both `RElement` and `RText`.
Additionally no text was stored in `TNode.value`. The result was that
the whole template could not be reconstructed. This refactoring creates
`TNodeType.Text` and store the text value in `TNode.value`.

The refactoring also changes `TNodeType` into flag-like structure make
it more efficient to check many different types at once.

PR Close #39233
alxhub added a commit that referenced this issue Oct 22, 2020
@mhevery @alxhub
refactor(core): Rename debugMatch to matchDebug for consistency (#…
cd2bf96 
…39233)

Previous function name `debugMatch` was not consistent with other match
functions.

PR Close #39233
alxhub added a commit that referenced this issue Oct 22, 2020
@mhevery @alxhub
refactor(core): rename COMMENT_MARKER to ICU_MARKER (#39233)
3fcbdc5 
`COMMENT_MARKER` is a generic name which does not make it obvious that
it is used for ICU use case. `ICU_MARKER` is more explicit as it is used
exclusively with ICUs.

PR Close #39233
alxhub added a commit that referenced this issue Oct 22, 2020
@mhevery @alxhub
refactor(core): cleanup i18n/icu data structures (#39233)
f2fea8c 
- Made `*OpCodes` array branded for safer type checking.
- Simplify `I18NRemoveOpCodes` encoding.
- Broke out  `IcuCreateOpCodes` from `I18nMutableOpCodes`.

PR Close #39233
alxhub added a commit that referenced this issue Oct 22, 2020
@mhevery @alxhub
refactor(core): Consistent use of HEADER_OFFSET (in ɵɵ* instructi…
61e98f8 
…ons only) (#39233)

IMPORTANT: `HEADER_OFFSET` should only be refereed to the in the `ɵɵ*` instructions to translate
instruction index into `LView` index. All other indexes should be in the `LView` index space and
there should be no need to refer to `HEADER_OFFSET` anywhere else.

PR Close #39233
alxhub added a commit that referenced this issue Oct 22, 2020
@mhevery @alxhub
refactor(core): Use ~x instead of -x which can result in -0 (#3…
a92b531 
…9233)

`expandoInstructions` uses negative numbers by `-x`. This has lead to
issues in the paste as `-0` is processed as float rather than integer
leading to de-optimization.

PR Close #39233
alxhub added a commit that referenced this issue Oct 22, 2020
@mhevery @alxhub
refactor(core): clean up circular dependencies (#39233)
13f9120 
Moved code from `interfaces/i18n.ts` which was causing circular dependencies

PR Close #39233
@alxhub alxhub closed this in 61d56be Oct 22, 2020
alxhub added a commit that referenced this issue Oct 22, 2020
@mhevery @alxhub
refactor(core): move i18n_spec.ts into i18n subfolder (#39233)
6790848 
`i18n_spec.ts` file was incorrectly in the `render3` folder rather than `render3/i18n`

PR Close #39233
alxhub added a commit that referenced this issue Oct 22, 2020
@mhevery @alxhub
fix(core): Store ICU state in LView rather than in TView (#39233)
ca11ef2 
Before this refactoring/fix the ICU would store the current selected
index in `TView`. This is incorrect, since if ICU is in `ngFor` it will
cause issues in some circumstances. This refactoring properly moves the
state to `LView`.

closes #37021
closes #38144
closes #38073

PR Close #39233
alxhub added a commit that referenced this issue Oct 22, 2020
@mhevery @alxhub
refactor(core): Change TName.tagName to a more generic value name. (
2e237ab 
#39233)

This is a pre-requisite for making the `TNode.value` a generic storage
mechanism for attaching data to `TNode`.

PR Close #39233
alxhub added a commit that referenced this issue Oct 22, 2020
@mhevery @alxhub
refactor(core): Remove hack where we TIcu was stored in tagName (#…
d50df92 
…39233)

Remove casting where we stored `TIcu` in `TNode.tagName` which was of
type `string` rather than `TIcu'. (renamed to `TNode.value` in previous
commit.)

PR Close #39233
alxhub added a commit that referenced this issue Oct 22, 2020
@mhevery @alxhub
refactor(core): Create TNodeType.Text to display full template in `…
70f1e2e 
…TView` debug (#39233)

When looking at `TView` debug template only Element nodes were displayed
as `TNode.Element` was used for both `RElement` and `RText`.
Additionally no text was stored in `TNode.value`. The result was that
the whole template could not be reconstructed. This refactoring creates
`TNodeType.Text` and store the text value in `TNode.value`.

The refactoring also changes `TNodeType` into flag-like structure make
it more efficient to check many different types at once.

PR Close #39233
alxhub added a commit that referenced this issue Oct 22, 2020
@mhevery @alxhub
refactor(core): Rename debugMatch to matchDebug for consistency (#…
1b9193b 
…39233)

Previous function name `debugMatch` was not consistent with other match
functions.

PR Close #39233
alxhub added a commit that referenced this issue Oct 22, 2020
@mhevery @alxhub
refactor(core): rename COMMENT_MARKER to ICU_MARKER (#39233)
e1f80d7 
`COMMENT_MARKER` is a generic name which does not make it obvious that
it is used for ICU use case. `ICU_MARKER` is more explicit as it is used
exclusively with ICUs.

PR Close #39233
alxhub added a commit that referenced this issue Oct 22, 2020
@mhevery @alxhub
refactor(core): cleanup i18n/icu data structures (#39233)
bc5005e 
- Made `*OpCodes` array branded for safer type checking.
- Simplify `I18NRemoveOpCodes` encoding.
- Broke out  `IcuCreateOpCodes` from `I18nMutableOpCodes`.

PR Close #39233
alxhub added a commit that referenced this issue Oct 22, 2020
@mhevery @alxhub
refactor(core): Consistent use of HEADER_OFFSET (in ɵɵ* instructi…
5430368 
…ons only) (#39233)

IMPORTANT: `HEADER_OFFSET` should only be refereed to the in the `ɵɵ*` instructions to translate
instruction index into `LView` index. All other indexes should be in the `LView` index space and
there should be no need to refer to `HEADER_OFFSET` anywhere else.

PR Close #39233
alxhub added a commit that referenced this issue Oct 22, 2020
@mhevery @alxhub
refactor(core): Use ~x instead of -x which can result in -0 (#3…
2c31533 
…9233)

`expandoInstructions` uses negative numbers by `-x`. This has lead to
issues in the paste as `-0` is processed as float rather than integer
leading to de-optimization.

PR Close #39233
alxhub added a commit that referenced this issue Oct 22, 2020
@mhevery @alxhub
refactor(core): clean up circular dependencies (#39233)
08f3d62 
Moved code from `interfaces/i18n.ts` which was causing circular dependencies

PR Close #39233
@AndrewKushnir AndrewKushnir mentioned this pull request Oct 23, 2020
Closed
3 tasks
@angular-automatic-lock-bot
Copy link

@angular-automatic-lock-bot angular-automatic-lock-bot bot commented Nov 22, 2020

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 Nov 22, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@alfaproject alfaproject

@petebacondarwin petebacondarwin

@IgorMinar IgorMinar

@jelbourn jelbourn

@alxhub alxhub

@AndrewKushnir AndrewKushnir

Assignees
No one assigned
Labels
action: merge cla: yes comp: i18n target: rc
Projects
None yet
Milestone
needsTriage
Linked issues

Successfully merging this pull request may close these issues.

None yet

8 participants
@mhevery @AndrewKushnir @IgorMinar @petebacondarwin @alfaproject @jelbourn @alxhub @atscott