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): i18n compiler - i18nStart and i18nEnd support #26442

Closed

Conversation

AndrewKushnir
Copy link
Contributor

PR Description

This PR introduces i18nStart and i18nEnd instructions support in i18n compiler, according to the I18n Design Doc. The main idea is to maintain an instance of I18nContext between start and end instructions, which keeps track of all i18n-related aspects (accumulates content, bindings, etc). When we enter a nested template (for ex. as a result of *ngIf), the top-level context is being passed down to the nested component, which uses this context to generate a child instance of I18nContext class (to handle nested template) and at the end, reconciles it back with the parent context.

Notes

  • <ng-template i18n>, <ng-content i18n> and <ng-container i18n> are not supported yet (coming next)
  • the logic of goog.getMsg generation remained as is (was not changed in this PR)
  • no ICU support introduced in this PR (coming next)

Next steps (in followup PRs)

  • <ng-*> tags support
  • updates to tests logic to support � symbol in templates
  • (to be discussed) ConstantPool refactoring to provide API to support Translation needs. Currently (historically) Translation-related logic to generate constants, is located in ConstantPool - we can probably extract it out to i18n.ts.
  • (to be discussed) introduce i18n instruction in case there is no other content in between i18nStart and i18nEnd, a-la element instruction

PR Type

[ ] Bugfix
[x] Feature
[ ] Code style update (formatting, local variables)

Does this PR introduce a breaking change?

[ ] Yes
[x] 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 target: major This PR is targeted for the next major release comp: ivy labels Oct 15, 2018
@mary-poppins
Copy link

You can preview f5a6ef5 at https://pr26442-f5a6ef5.ngbuilds.io/.

const $r2$ = {"i":[13]};
`;
const template = String.raw `
const $MSG_APP_SPEC_TS_0$ = goog.getMsg("\uFFFD#0\uFFFDMy i18n block #1\uFFFD/#0\uFFFD");
Copy link
Contributor

Choose a reason for hiding this comment

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

If I'm not mistaken, the div with the i18n attribute should not be in the generated message: \uFFFD#0\uFFFDMy i18n block #1\uFFFD/#0\uFFFD --> My i18n block #1. We only want what's inside that element.
Same thing for all examples below.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good catch, thanks! I've updated the code and the necessary tests to handle that.

};

const template = String.raw `
const $MSG_APP_SPEC_TS_0$ = goog.getMsg("\uFFFD#0\uFFFD My i18n block #\uFFFD0\uFFFD \uFFFD#2\uFFFDPlain text in nested element\uFFFD/#2\uFFFD\uFFFD/#0\uFFFD");
Copy link
Contributor

Choose a reason for hiding this comment

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

There an extra space generated at the beginning: \uFFFD#0\uFFFD My i18n block (before My). This might be due to the line break, but we should not generate this. I think that the messages should be extracted with the option "whitespace removal" activated.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Agree, I've added the code to trim the content while assembling i18n payload.

@mary-poppins
Copy link

You can preview 471f857 at https://pr26442-471f857.ngbuilds.io/.

@AndrewKushnir
Copy link
Contributor Author

@JoostK thanks for your comments! 👍 I've performed the necessary updates.

@mary-poppins
Copy link

You can preview 6002c8f at https://pr26442-6002c8f.ngbuilds.io/.

@mary-poppins
Copy link

You can preview e248127 at https://pr26442-e248127.ngbuilds.io/.

if (rf & 1) {
$r3$.ɵelementStart(0, "div");
$r3$.ɵi18nStart(1, $MSG_APP_SPEC_TS_0$);
$r3$.ɵelementStart(2, "span");
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
$r3$.ɵelementStart(2, "span");
$r3$.ɵelement(2, "span");

Copy link
Contributor

Choose a reason for hiding this comment

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

also consider i18n(..) which merges start/end

$r3$.ɵelementStart(0, "div");
$r3$.ɵi18nStart(1, $MSG_APP_SPEC_TS_0$);
$r3$.ɵelementStart(2, "span");
$r3$.ɵelementEnd();
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
$r3$.ɵelementEnd();

const $msg_1$ = goog.getMsg("Hello world");
const $msg_2$ = goog.getMsg("farewell");
/**
* @desc descA
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
* @desc descA
* @desc [BACKUP_MESSAGE_ID:idA] descA

packages/compiler/src/render3/view/i18n.ts Show resolved Hide resolved
@mhevery mhevery added the action: cleanup The PR is in need of cleanup, either due to needing a rebase or in response to comments from reviews label Oct 16, 2018
@mary-poppins
Copy link

You can preview a4cda7d at https://pr26442-a4cda7d.ngbuilds.io/.

@mary-poppins
Copy link

You can preview fe2b743 at https://pr26442-fe2b743.ngbuilds.io/.

@mary-poppins
Copy link

You can preview e58233d at https://pr26442-e58233d.ngbuilds.io/.

@AndrewKushnir AndrewKushnir added action: merge The PR is ready for merge by the caretaker 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 Oct 17, 2018
jbogarthyde pushed a commit to jbogarthyde/angular that referenced this pull request Oct 17, 2018
@mhevery mhevery closed this in 8024857 Oct 17, 2018
sculove pushed a commit to sculove/angular that referenced this pull request Nov 2, 2018
sculove pushed a commit to sculove/angular that referenced this pull request Nov 2, 2018
FrederikSchlemmer pushed a commit to FrederikSchlemmer/angular that referenced this pull request Jan 3, 2019
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

6 participants