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(compiler): correctly parse attributes with a dot in the name #32256

Closed
wants to merge 1 commit into from

Conversation

@n0th1ng-else
Copy link
Contributor

n0th1ng-else commented Aug 22, 2019

Previously the compiler would ignore everything in the attribute
name after the first dot. For example
<div [attr.someAttr.attrSuffix]="var"></div>
is turned into <div someAttr="varValue"></div>.

This commit ensures that whole attribute name is captured.
Now <div [attr.someAttr.attrSuffix]="var"></div>
is turned into <div someAttr.attrSuffix="varValue"></div>

PR Checklist

Please check if your PR fulfills the following requirements:

PR Type

What kind of change does this PR introduce?

  • Bugfix
  • Feature
  • Code style update (formatting, local variables)
  • Refactoring (no functional changes, no api changes)
  • Build related changes
  • CI related changes
  • Documentation content changes
  • angular.io application / infrastructure changes
  • Other... Please describe:

What is the current behavior?

element
<div [attr.someProp.propSuffix]="var"></div>
turns into
<div someProp="varValue"></div> (note the propSuffix is cut)

Issue Number: #31334

What is the new behavior?

element
<div [attr.someProp.propSuffix]="var"></div>
result this html
<div someProp.propSuffix="varValue"></div> (note the propSuffix now exists)

Does this PR introduce a breaking change?

  • Yes
  • No

Other information

@n0th1ng-else n0th1ng-else requested a review from angular/fw-compiler as a code owner Aug 22, 2019
@googlebot googlebot added the cla: yes label Aug 22, 2019
@n0th1ng-else n0th1ng-else force-pushed the n0th1ng-else:fix-attr-bind branch from 0ef5f94 to 6162390 Aug 22, 2019
@ngbot ngbot bot added this to the needsTriage milestone Aug 22, 2019
Copy link
Member

petebacondarwin left a comment

LGTM - can you modify the commit message slightly:

fix(compiler): correctly parse attributes with a dot in the name

Previously the compiler would ignore everything in the attribute
name after the first dot. For example
<div [attr.someAttr.attrSuffix]="var"></div>
is turned into <div someAttr="varValue"></div>.

This commit ensures that whole attribute name is captured.
Now <div [attr.someAttr.attrSuffix]="var"></div>
is turned into <div someAttr.attrSuffix="varValue"></div>.

@n0th1ng-else n0th1ng-else force-pushed the n0th1ng-else:fix-attr-bind branch from 6162390 to a74b239 Nov 5, 2019
Previously the compiler would ignore everything in the attribute
name after the first dot. For example
<div [attr.someAttr.attrSuffix]="var"></div>
is turned into <div someAttr="varValue"></div>.

This commit ensures that whole attribute name is captured.
Now <div [attr.someAttr.attrSuffix]="var"></div>
is turned into <div someAttr.attrSuffix="varValue"></div>
@n0th1ng-else n0th1ng-else force-pushed the n0th1ng-else:fix-attr-bind branch from a74b239 to 815afb3 Nov 5, 2019
@n0th1ng-else n0th1ng-else changed the title fix(compiler): Fix parsing attributes with dot in the name fix(compiler): correctly parse attributes with a dot in the name Nov 5, 2019
@n0th1ng-else

This comment has been minimized.

Copy link
Contributor Author

n0th1ng-else commented Nov 5, 2019

@petebacondarwin thank you! I have updated the commit message 👍

@alxhub
alxhub approved these changes Nov 5, 2019
@ngbot

This comment has been minimized.

Copy link

ngbot bot commented Nov 6, 2019

I see that you just added the PR action: merge label, but the following checks are still failing:
    failure status "google3" is failing

If you want your PR to be merged, it has to pass all the CI checks.

If you can't get the PR to a green state due to flakes or broken master, please try rebasing to master and/or restarting the CI job. If that fails and you believe that the issue is not due to your change, please contact the caretaker and ask for help.

@n0th1ng-else

This comment has been minimized.

Copy link
Contributor Author

n0th1ng-else commented Nov 6, 2019

Is it something I need to do from my side, @petebacondarwin? Not sure I can understand the falling status correctly

@petebacondarwin

This comment has been minimized.

Copy link
Member

petebacondarwin commented Nov 6, 2019

@n0th1ng-else - no we just need a Googler (perhaps @alxhub ?) to run a presubmit to test that it works inside Google's repository before it can be merged.

@atscott

This comment has been minimized.

Copy link
Contributor

atscott commented Nov 6, 2019

@atscott

This comment has been minimized.

Copy link
Contributor

atscott commented Nov 6, 2019

Hi @n0th1ng-else, this seems to break [attr.height.px]="16". Previously, this would result in height="16px" but now you get height.px="16".

@petebacondarwin

This comment has been minimized.

Copy link
Member

petebacondarwin commented Nov 6, 2019

Hmm, that is a bit of a problem. I'm surprised that we don't have tests for this in the framework!

The height.px thing is an Angular custom syntax, but attribute names with dots in them are valid.

It might be that we can avoid dotted names for some subset of well-known attributes, like height?

@atscott

This comment has been minimized.

Copy link
Contributor

atscott commented Nov 6, 2019

Ah, sorry this was incorrect. After some digging, what actually happened was an incorrect configuration in the failing component. It has [attr.height.px]="16" on an svg element. The way it's working right now, this .px was ignored so the result was height="16". This is correct for an svg element, but this change made it height.px="16" which is not correct. So the component's template actually needs to be updated. @alxhub - does this need to be added to the breaking changes list? It seems like there may potentially be cases in VE that are relying on broken behavior.

@alxhub

This comment has been minimized.

Copy link
Contributor

alxhub commented Nov 6, 2019

I think we should push this to 10.0 unfortunately. We're in RC now, I don't think we should introduce more breaking changes unless there are issues with the current RC that we cannot address otherwise. We could potentially investigate doing this as a fix if we can find a way to make it non-breaking.

@petebacondarwin

This comment has been minimized.

Copy link
Member

petebacondarwin commented Nov 6, 2019

I don't think 9.1 can accept breaking changes...

@ngbot

This comment has been minimized.

Copy link

ngbot bot commented Nov 7, 2019

I see that you just added the PR action: merge label, but the following checks are still failing:
    failure status "google3" is failing

If you want your PR to be merged, it has to pass all the CI checks.

If you can't get the PR to a green state due to flakes or broken master, please try rebasing to master and/or restarting the CI job. If that fails and you believe that the issue is not due to your change, please contact the caretaker and ask for help.

@atscott

This comment has been minimized.

Copy link
Contributor

atscott commented Nov 7, 2019

Discussed with @alxhub and given that we only found one occurrence for [attr.attribute.suffix] in google3 and we've fixed it in preparation for this PR, we're okay to merge.

@atscott atscott closed this in c0f69f3 Nov 7, 2019
atscott added a commit that referenced this pull request Nov 7, 2019
)

Previously the compiler would ignore everything in the attribute
name after the first dot. For example
<div [attr.someAttr.attrSuffix]="var"></div>
is turned into <div someAttr="varValue"></div>.

This commit ensures that whole attribute name is captured.
Now <div [attr.someAttr.attrSuffix]="var"></div>
is turned into <div someAttr.attrSuffix="varValue"></div>

PR Close #32256
mohaxspb added a commit to mohaxspb/angular that referenced this pull request Nov 7, 2019
…ular#32256)

Previously the compiler would ignore everything in the attribute
name after the first dot. For example
<div [attr.someAttr.attrSuffix]="var"></div>
is turned into <div someAttr="varValue"></div>.

This commit ensures that whole attribute name is captured.
Now <div [attr.someAttr.attrSuffix]="var"></div>
is turned into <div someAttr.attrSuffix="varValue"></div>

PR Close angular#32256
mohaxspb added a commit to mohaxspb/angular that referenced this pull request Nov 7, 2019
…ular#32256)

Previously the compiler would ignore everything in the attribute
name after the first dot. For example
<div [attr.someAttr.attrSuffix]="var"></div>
is turned into <div someAttr="varValue"></div>.

This commit ensures that whole attribute name is captured.
Now <div [attr.someAttr.attrSuffix]="var"></div>
is turned into <div someAttr.attrSuffix="varValue"></div>

PR Close angular#32256
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.