Formed in 2009, the Archive Team (not to be confused with the archive.org Archive-It Team) is a rogue archivist collective dedicated to saving copies of rapidly dying or deleted websites for the sake of history and digital heritage. The group is 100% composed of volunteers and interested parties, and has expanded into a large amount of related projects for saving online and digital history.
fix(compiler): treat i18n attributes with no bindings as static attributes #39408
Conversation
…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.
Looks great, thanks for this PR @crisbeto
I think we can also simplify the i18nAttributes instruction by removing the code that takes care of the static value processing here. There might be even more refactoring in that function that we can look at: we may not need the hasBinding check (since we know that it's a dynamic value, this we'd have bindings) and ICU-related logic there (we do not support ICUs in attrs atm).
crisbeto
force-pushed
the
38231/static-i18n-attrs
branch
from
79c6211
to
61bddae
Compare
Thanks for the feedback @AndrewKushnir, all of it should be addressed now. Also something I was thinking about while addressing the feedback: would it make sense to rename the i18nAttribute instruction to something like i18nDynamicAttribute to be more explicit about its uses? It doesn't have to happen as a part of this PR.
Just left a couple comments, please see below.
Also something I was thinking about while addressing the feedback: would it make sense to rename the i18nAttribute instruction to something like i18nDynamicAttribute to be more explicit about its uses?
Yeah, that makes sense, let's have a followup PR with the rename (once this PR lands). The "dynamic" sounds good, I was also thinking if we could call it "bound" (i.e. i18nBoundAttributes), which might be more aligned with the compiler terminology (where we have t.BoundAttribute).
|
+1 for |
Nice bit of refactoring. I agree with @AndrewKushnir's comments and added a few more.
Thanks for jumping on this @crisbeto!
packages/compiler-cli/test/compliance/r3_view_compiler_i18n_spec.ts
Outdated
Show resolved
Hide resolved
|
I've addressed the latest set of feedback. |
Thanks for the updates @crisbeto
It's really great to see how the i18nAttributesFirstPass function is now simplified!
|
Merging to RC for now - please open a separate patch PR if this needs to merge to the patch branch. |
…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
This is follow-up from [an earlier discussion](angular#39408 (comment)). After some testing, it looks like the type of `Element.attributes` was correct in specifying that it only has `TextAttribute` instances. This means that the extra checks that filter out `BoundAttribute` instances from the array isn't necessary. There is another loop a bit further down that actually extracts the bound i18n attributes.
This is follow-up from [an earlier discussion](#39408 (comment)). After some testing, it looks like the type of `Element.attributes` was correct in specifying that it only has `TextAttribute` instances. This means that the extra checks that filter out `BoundAttribute` instances from the array isn't necessary. There is another loop a bit further down that actually extracts the bound i18n attributes. PR Close #39498
This is follow-up from [an earlier discussion](#39408 (comment)). After some testing, it looks like the type of `Element.attributes` was correct in specifying that it only has `TextAttribute` instances. This means that the extra checks that filter out `BoundAttribute` instances from the array isn't necessary. There is another loop a bit further down that actually extracts the bound i18n attributes. PR Close #39498
|
This issue has been automatically locked due to inactivity. Read more about our automatic conversation locking policy. This action has been performed automatically by a bot. |
Currently
i18nattributes 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ɵɵi18nAttributesinstruction and prevents the translated attributes from being injected using the@Attributedecorator.These changes makes it so that static translated attributes are treated in the same way as regular static attributes and all other
i18nattributes go through the old code path.Fixes #38231.