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

fix(ivy): Ensure ngProjectAs marker name appears at even attribute index #34617

Closed
wants to merge 5 commits into from

Conversation

@atscott
Copy link
Contributor

atscott commented Jan 2, 2020

The getProjectAsAttrValue in node_selector_matcher finds the
ProjectAs marker and then additionally checks that the marker appears in
an even index of the node attributes because "attribute names are stored
at even indexes". This is true for "regular" attribute bindings but
classes, styles, bindings, templates, and i18n do not necessarily follow
this rule because there can be an uneven number of them, causing the
next "special" attribute "name" to appear at an odd index. To address
this issue, ensure ngProjectAs is placed right after "regular"
attributes.

The `getProjectAsAttrValue` in `node_selector_matcher` finds the
ProjectAs marker and then additionally checks that the marker appears in
an even index of the node attributes because "attribute names are stored
at even indexes". This is true for "regular" attribute bindings but
classes, styles, bindings, templates, and i18n do not necessarily follow
this rule because there can be an uneven number of them, causing the
next "special" attribute "name" to appear at an odd index. To address
this issue, ensure ngProjectAs is placed right after "regular"
attributes.
@atscott atscott requested review from alxhub and crisbeto Jan 2, 2020
@atscott atscott requested review from angular/fw-compiler as code owners Jan 2, 2020
@googlebot googlebot added the cla: yes label Jan 2, 2020
@@ -1311,10 +1319,6 @@ export class TemplateDefinitionBuilder implements t.Visitor<void>, LocalResolver
templateAttrs.forEach(attr => addAttrExpr(attr.name));
}

if (ngProjectAsAttr) {

This comment has been minimized.

Copy link
@crisbeto

crisbeto Jan 3, 2020

Member

Maybe we can keep the this logic inside prepareNonRenderAttrs by moving the push further up? That way we can keep all the logic for the non-render attributes together.

This comment has been minimized.

Copy link
@atscott

atscott Jan 3, 2020

Author Contributor

I was waffling back and forth on this one. I moved it out because having it inside sort of makes some assumptions about the context in which the function is called that the function doesn't necessarily know about. It felt like the only real way to guarantee placement right after the other k/v attribute pairs was to have the code next to each other.

I did move it back though to keep it with the other non-render attributes.

This comment has been minimized.

Copy link
@AndrewKushnir

AndrewKushnir Jan 3, 2020

Contributor

As an option, we can refactor the code a bit and extend the function in template.ts that assembles attribute array (which is currently called prepareNonRenderAttrs, but we may need to find a better name during refactoring) to also accepts set of attributes (that should be at the very beginning of an array) as an argument. In this case, the right order/shape of the attributes array would be encapsulated within the prepareNonRenderAttrs function. We can reuse this function in the visitContent function as well for consistency.

This comment has been minimized.

Copy link
@atscott

atscott Jan 6, 2020

Author Contributor

Refactored to single function based on suggestion from @AndrewKushnir

Copy link
Member

crisbeto left a comment

LGTM

@ngbot ngbot bot modified the milestone: needsTriage Jan 3, 2020
@atscott atscott force-pushed the atscott:ngprojectas branch 2 times, most recently from 00cecd3 to b080f66 Jan 6, 2020
@atscott atscott force-pushed the atscott:ngprojectas branch from b080f66 to 65acc67 Jan 6, 2020
Copy link
Contributor

AndrewKushnir left a comment

LGTM 👍

@alxhub
alxhub approved these changes Jan 6, 2020
@atscott

This comment has been minimized.

Copy link
Contributor Author

atscott commented Jan 6, 2020

alxhub added a commit that referenced this pull request Jan 7, 2020
…dex (#34617)

The `getProjectAsAttrValue` in `node_selector_matcher` finds the
ProjectAs marker and then additionally checks that the marker appears in
an even index of the node attributes because "attribute names are stored
at even indexes". This is true for "regular" attribute bindings but
classes, styles, bindings, templates, and i18n do not necessarily follow
this rule because there can be an uneven number of them, causing the
next "special" attribute "name" to appear at an odd index. To address
this issue, ensure ngProjectAs is placed right after "regular"
attributes.

PR Close #34617
@alxhub alxhub closed this in 4d7a9db Jan 7, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.