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

feat(ivy): ICU support for Ivy (compiler side) #26794

Closed

Conversation

AndrewKushnir
Copy link
Contributor

@AndrewKushnir AndrewKushnir commented Oct 27, 2018

The goal of this PR is to add I18n ICU support to IVY compiler. In order to satisfy the backwards (and forward) compatibility requirement, the logic of producing translation messages (goog.getMsg at this moment) mimics existing ng xi18n machinery output, specifically:

  • each ICU takes a separate translation call/unit
  • nested ICUs are not presented as top-level translations, they remain a part of the parent ICU
  • attribute translation also takes a separate call/unit

The I18n AST (which is used to power ng xi18n mechanics) was employed to minimize discrepancies between the ng xi18n output and IVY compiler output. Nodes within I18n sections now have references to their canonical representation in i18n AST, which helps maintain output consistency.

Update (11/8)
As a part of this PR, the following logic was implemented:

  • post-processing for goog.getMsg output, since the placeholders might not be unique (for ex. CLOSE_TAG_DIV can be used multiple times, but we have to have different references/indices). Right now in such cases we put a specially-formatted string that contains these combinations (for ex. [�/#1�|�/#4:3�|�7:4�]). The post-processing step assembles the final string at runtime, based on this information.
  • the ConstanlPool was refactored and all i18n-related code was moved away
  • added support for translations without root element (e.g. <ng-container i18n>)
  • added support for <ng-template i18n>
  • support for goog.getMsg was improved to generate compatible translation strings and respective placeholders in appropriate format
  • implemented i18n self-closing instruction (in case there are no elements between i18nStart and i18nEnd)

This PR now completes the main set of features required for i18n compiler.

PR Type

What kind of change does this PR introduce?

  • Bugfix
  • Feature
  • Code style update (formatting, local variables)

Does this PR introduce a breaking change?

  • Yes
  • No

@AndrewKushnir AndrewKushnir added feature Issue that requests a new feature action: review The PR is still awaiting reviews from at least one requested reviewer comp: ivy labels Oct 27, 2018
@ngbot ngbot bot added this to the needsTriage milestone Oct 27, 2018
Copy link
Contributor

@mhevery mhevery left a comment

Choose a reason for hiding this comment

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

I would like @alxhub to have a look at the compiler details as he is more familiar with it. Left comments on the generated code.

Copy link
Contributor

@ocombe ocombe left a comment

Choose a reason for hiding this comment

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

A few things to change, see my inline comments.
And check with @mhevery if we should apply the whitespace removal, and also if we should keep empty translations or not.

@ocombe

This comment has been minimized.

@AndrewKushnir AndrewKushnir force-pushed the FW-530_i18n-icu-support branch 2 times, most recently from 782184c to aba5e42 Compare November 6, 2018 00:52
@AndrewKushnir
Copy link
Contributor Author

@ocombe @mhevery thanks for the initial review!
I've addressed your comments (and added some additional features). I plan to add extra tests and docs for utils functions, but generally this PR is ready for another review, when you get a chance.
Thank you.

@AndrewKushnir AndrewKushnir force-pushed the FW-530_i18n-icu-support branch 2 times, most recently from 9699edf to 5497d11 Compare November 6, 2018 20:08
Copy link
Member

@alxhub alxhub left a comment

Choose a reason for hiding this comment

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

First round of comments.

In general, I notice that <element>.i18n ! happens a lot - we're constantly asserting in this PR that i18n fields are not null. I think this indicates a more fundamental issue with the typings. Can you think of any way to avoid this? I've not really studied the problem deeply but I do have one observation which might be helpful.

Part of the problem comes up at the boundary between i18n-processing code and the larger TemplateDefinitionBuilder. We do things like:

if (element.i18n) {
 // i18n field is known to be defined here.
 doSomethingWithElement(element);
}

function doSomethingWithElement(element: t.Element) {
  // element.i18n is not known to be defined, so assertion is required.
  console.log(element.i18n !);
}

Instead of this pattern, we could make functions on the boundary to which i18n-enabled nodes are passed have signatures like:

function doSomethingWithElement(element: t.Element & {i18n: I18nType})

This way:

  1. we can only call the function on a t.Element with a valid i18n field.
  2. we don't lose that information in the body of the function, and thus don't need the assertion.

This gets the type system working with us and not against us.

Make sense?

packages/compiler/src/constant_pool.ts Outdated Show resolved Hide resolved
packages/compiler/src/i18n/i18n_ast.ts Outdated Show resolved Hide resolved
packages/compiler/src/i18n/i18n_parser.ts Outdated Show resolved Hide resolved
packages/compiler/src/render3/r3_identifiers.ts Outdated Show resolved Hide resolved
packages/compiler/src/render3/r3_template_transform.ts Outdated Show resolved Hide resolved
packages/compiler/src/render3/view/i18n/context.ts Outdated Show resolved Hide resolved
packages/compiler/src/render3/view/i18n/meta.ts Outdated Show resolved Hide resolved
packages/compiler/src/render3/view/template.ts Outdated Show resolved Hide resolved
packages/compiler/src/render3/view/template.ts Outdated Show resolved Hide resolved
packages/compiler/src/render3/view/template.ts Outdated Show resolved Hide resolved
@angular angular deleted a comment from mary-poppins Nov 9, 2018
@angular angular deleted a comment from mary-poppins Nov 9, 2018
@angular angular deleted a comment from mary-poppins Nov 9, 2018
@angular angular deleted a comment from mary-poppins Nov 9, 2018
@angular angular deleted a comment from mary-poppins Nov 9, 2018
@angular angular deleted a comment from mary-poppins Nov 9, 2018
@angular angular deleted a comment from mary-poppins Nov 9, 2018
@angular angular deleted a comment from mary-poppins Nov 9, 2018
@angular angular deleted a comment from mary-poppins Nov 9, 2018
@angular angular deleted a comment from mary-poppins Nov 9, 2018
@angular angular deleted a comment from mary-poppins Nov 9, 2018
@googlebot
Copy link

A Googler has manually verified that the CLAs look good.

(Googler, please make sure the reason for overriding the CLA status is clearly documented in these comments.)

@mary-poppins
Copy link

You can preview a1bb4be at https://pr26794-a1bb4be.ngbuilds.io/.

@AndrewKushnir
Copy link
Contributor Author

@alxhub thanks for review!
I rebased this PR from the latest master that now includes the change from a different PR (60800da), which takes care of elementContainer-related changes (so my PR no longer contains this code). Thank you.

@angular angular deleted a comment from mary-poppins Nov 16, 2018
@angular angular deleted a comment from mary-poppins Nov 16, 2018
@angular angular deleted a comment from googlebot Nov 16, 2018
@AndrewKushnir
Copy link
Contributor Author

AndrewKushnir commented Nov 16, 2018

@angular angular deleted a comment from googlebot Nov 16, 2018
@AndrewKushnir AndrewKushnir added action: merge The PR is ready for merge by the caretaker target: major This PR is targeted for the next major release and removed action: cleanup The PR is in need of cleanup, either due to needing a rebase or in response to comments from reviews action: review The PR is still awaiting reviews from at least one requested reviewer labels Nov 16, 2018
@mhevery mhevery closed this in 92e80af Nov 17, 2018
FrederikSchlemmer pushed a commit to FrederikSchlemmer/angular that referenced this pull request Jan 3, 2019
@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.
Labels
action: merge The PR is ready for merge by the caretaker cla: yes feature Issue that requests a new feature target: major This PR is targeted for the next major release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

8 participants