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

i18n with ivy breaks the injection of translated attribute #38231

Closed
klemenoslaj opened this issue Jul 25, 2020 · 5 comments
Closed

i18n with ivy breaks the injection of translated attribute #38231

klemenoslaj opened this issue Jul 25, 2020 · 5 comments
Assignees
Labels
area: i18n freq1: low P2 The issue is important to a large percentage of users, with a workaround regression Indicates than the issue relates to something that worked in a previous version state: confirmed state: has PR type: bug/fix
Milestone

Comments

@klemenoslaj
Copy link
Contributor

🐞 bug report

Affected Package

The issue is caused by package @angular/localize.

Is this a regression?

Yes, this issue is reproducible only with Ivy. With ViewEngine it works fine.

Description

Injecting the attribute to the component/directive that is translated using i18n is always resulting in null.

Example:

text is a directive that reads the text attribute and simply adds the following text to the host element:

Text attribtue value is:

Code:

<div text="Text without i18n"></div>
<div i18n-text text="Text with i18n"></div>

Resulting HTML:

<div text="Text without i18n">Text attribtue value is: Text without i18n.</div>
<div text="Text with i18n">Text attribtue value is: null.</div>

Observed in the result above, text attribute is properly set to Text with i18n, while the injected value remains null.

🔬 Minimal Reproduction

https://stackblitz.com/edit/angular-ivy-uzgvwe

You can observe null as described above.

To make this example work, open settings and uncheck Enable Ivy. Now ViewEngine will be used to compile the application, and the null text will be replaced by Text with i18n.

🌍 Your Environment

Angular CLI: 10.0.4
Node: 12.16.3
OS: linux x64

Angular: 10.0.5
... animations, common, compiler, compiler-cli, core, forms
... localize, platform-browser, platform-browser-dynamic, router
Ivy Workspace: Yes

Package Version

@angular-devkit/architect 0.1000.4
@angular-devkit/build-angular 0.1000.4
@angular-devkit/build-optimizer 0.1000.4
@angular-devkit/build-webpack 0.1000.4
@angular-devkit/core 10.0.4
@angular-devkit/schematics 10.0.4
@angular/cli 10.0.4
@ngtools/webpack 10.0.4
@schematics/angular 10.0.4
@schematics/update 0.1000.4
rxjs 6.5.5
typescript 3.9.7
webpack 4.43.0

@ngbot ngbot bot added this to the needsTriage milestone Jul 25, 2020
@petebacondarwin
Copy link
Member

petebacondarwin commented Aug 12, 2020

Note that this involves an @Attribute decorator rather than an @Input decorator to pass the value through.
It does work if you use @Input instead: https://stackblitz.com/edit/angular-ivy-af37h8?file=src%2Fapp%2Ftext.directive.ts

@petebacondarwin petebacondarwin added engine: ivy regression Indicates than the issue relates to something that worked in a previous version state: confirmed type: bug/fix labels Aug 12, 2020
@ngbot ngbot bot modified the milestones: needsTriage, Backlog Sep 17, 2020
@jelbourn jelbourn added the P2 The issue is important to a large percentage of users, with a workaround label Oct 1, 2020
@petebacondarwin
Copy link
Member

Some further analysis... the generated ivy instructions for the template look like:

i0.ɵɵelement(0, "div", 0);

i0.ɵɵelementStart(1, "div", 1);
  i0.ɵɵi18nAttributes(2, 2);
i0.ɵɵelementEnd();

When this code executes the i18n messages have been computed and are stored in the constants array for this view:

tView.consts = [
  ["text", "Text without i18n"]
  [6, "text"]
  ["text", "Text with i18n"]
]

The call to element() is for the first <div>. It will instantiate the TextDirective and will pass through the correct values tView.consts[0], which is the name-value tuple ["text", "Text without i18n"] for its attribute injection.

The next call to ɵɵelementStart() is for the second <div>. It will also instantiate the TextDirective but the attribute associated with this element corresponds to an "i18n" attribute marked by the 6 in tView.consts[1] ([6, "text"]). The actual value of this attribute has not yet been determined since that happens in the next instruction ɵɵi18nAttributes(), which will use the final constant tView.consts[2] (["text", "Text with i18n"]).

@petebacondarwin
Copy link
Member

The ɵɵi18nAttributes() instruction is actually responsible for two things.

  • First it must get hold of the i18n message and process it, in case it contains dynamic elements such as interpolations.
  • Second it must attach the computed i18n message to the element as an attribute.

Clearly it cannot do the second part until after the element has been created (in ɵɵelementStart()) but that is also, unfortunately where the TextDirective gets instantiated. So that is why the directive is receiving null rather than the i18n message.

Thinking out loud: I wonder if we could somehow split the functionality of ɵɵi18nAttributes() so that it can prepare the attribute values before the element is created and then attach the attribute afterwards?
Something like:

i0.ɵɵi18nPrepareAttributes(1, 2);
i0.ɵɵelementStart(1, "div", 1);
i0.ɵɵi18nAttachAttributes(2, 2);
i0.ɵɵelementEnd();

@petebacondarwin
Copy link
Member

@AndrewKushnir and I discussed this. It turns out that we can probably do even better than splitting the instruction, as described above.

The point is that only attributes that hold static text can be injected into directives via the @Attribute decorator anyway.

At compile time we are able to work out if i18n messages are static (i.e. ones that have no placeholders for things like interpolation, ICUs, nested elements etc). In such cases there is no need to do any runtime i18n manipulation of the value, it should just be applied as a constant directly to the attribute just like a non-i18n attribute value.

This would solve this bug, but also allow us to remove invocations of i18n instructions for those cases, making the generated code smaller and the runtime faster.

So to be more concrete, the generated could would now have the following constants array:

  ["text", "Text without i18n"]
  ["text", I18N_0]
]

where the I18N_0variable would hold the result of the $localize tagged string (or the replaced string if inlined at compile time.

And the instructions for this scenario would simply be:

i0.ɵɵelement(0, "div", 0);
i0.ɵɵelement(1, "div", 1);

Obviously in cases where the i18n message has dynamic elements, we would use the current instructions, which would preclude the value from being injected via @Attribute() but this is the case already, since things like interpolation cannot be computed at element creation time.

@crisbeto crisbeto self-assigned this Oct 24, 2020
crisbeto added a commit to crisbeto/angular that referenced this issue Oct 24, 2020
…butes

Currently `i18n` attributes are treated the same no matter if they have data bindings or not. This
both generates more code since they have to go through the `ɵɵi18nAttributes` instruction and
prevents the translated attributes from being injected using the `@Attribute` decorator.

These changes makes it so that static translated attributes are treated in the same way as regular
static attributes and all other `i18n` attributes go through the old code path.

Fixes angular#38231.
alxhub pushed a commit that referenced this issue Oct 27, 2020
…butes (#39408)

Currently `i18n` attributes are treated the same no matter if they have data bindings or not. This
both generates more code since they have to go through the `ɵɵi18nAttributes` instruction and
prevents the translated attributes from being injected using the `@Attribute` decorator.

These changes makes it so that static translated attributes are treated in the same way as regular
static attributes and all other `i18n` attributes go through the old code path.

Fixes #38231.

PR Close #39408
@alxhub alxhub closed this as completed in 58408d6 Oct 27, 2020
@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 Nov 27, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area: i18n freq1: low P2 The issue is important to a large percentage of users, with a workaround regression Indicates than the issue relates to something that worked in a previous version state: confirmed state: has PR type: bug/fix
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants