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.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Host bindings generate invalid code for quoted property names with dashes #40230

Closed
crisbeto opened this issue Dec 22, 2020 · 1 comment
Closed

Host bindings generate invalid code for quoted property names with dashes #40230

crisbeto opened this issue Dec 22, 2020 · 1 comment
Assignees
@crisbeto
Labels
area: compiler Issues related to `ngc`, Angular's template compiler area: core Issues related to the framework runtime P4 A relatively minor issue that is not relevant to core functions state: confirmed state: has PR
Milestone
Backlog

Comments

@crisbeto
Copy link
Member

馃悶 bug report

Affected Package

The issue is caused by package @angular/....

@angular/compiler-cli

Is this a regression?

It's not a regression, it seems to have been there since the early days of the framework.

Description

When the compiler is analyzing the class members decorated with @HostBinding, it takes the property name as is and adds it to the analysis result. Further down the compilation pipeline, we try to evaluate the value to determine whether it is a primitive or a reference to something on the component class. For HostBinding specifically it is always a reference, but it can get mixed up, because we use the same logic for the HostBinding decorator and the host object. The root cause is identical to what is happening in #40220.

Here's an example of the TS source code and the transpiled expression:

// Source
@HostBinding('class.some-class') "foo-bar": boolean;

// Result
傻傻classProp("some-class", ctx.foo - ctx.bar);

Note how the property name is actually treated as subtraction.
crisbeto added a commit to crisbeto/angular that referenced this issue Dec 22, 2020
@crisbeto
fix(compiler): incorrectly interpreting some HostBinding names
75e69fb 
Currently when analyzing the metadata of a directive, we bundle together the bindings from `host`
and the `HostBinding` and `HostListener` together. This can become a problem later on in the
compilation pipeline, because we try to evaluate the value of the binding, causing something like
`@HostBinding('class.foo') public true = 1;` to be treated the same as
`host: {'[class.foo]': 'true'}`.

While looking into the issue, I noticed another one that is closely related: we weren't treating
quoted property names correctly. E.g. `@HostBinding('class.foo') public "foo-bar" = 1;` was being
interpreted as `classProp('foo', ctx.foo - ctx.bar)` due to the same issue where property names
were being evaluated.

These changes resolve both of the issues by treating all `HostBinding` instance as if they're
reading the property from `this`. E.g. the `@HostBinding('class.foo') public true = 1;` from above
is now being treated as `host: {'[class.foo]': 'this.true'}` which further down the pipeline becomes
`classProp('foo', ctx.true)`. This doesn't have any payload size implications for existing code,
because we've always been prefixing implicit property reads with `ctx.`. If the property doesn't
have an identifier that can be read using dotted access, we convert it to a quoted one (e.g.
`classProp('foo', ctx['is-foo']))`.

Fixes angular#40220.
Fixes angular#40230.
@crisbeto crisbeto mentioned this issue Dec 22, 2020
Closed
crisbeto added a commit to crisbeto/angular that referenced this issue Dec 22, 2020
@crisbeto
fix(compiler): incorrectly interpreting some HostBinding names
c0b0c1f 
Currently when analyzing the metadata of a directive, we bundle together the bindings from `host`
and the `HostBinding` and `HostListener` together. This can become a problem later on in the
compilation pipeline, because we try to evaluate the value of the binding, causing something like
`@HostBinding('class.foo') public true = 1;` to be treated the same as
`host: {'[class.foo]': 'true'}`.

While looking into the issue, I noticed another one that is closely related: we weren't treating
quoted property names correctly. E.g. `@HostBinding('class.foo') public "foo-bar" = 1;` was being
interpreted as `classProp('foo', ctx.foo - ctx.bar)` due to the same issue where property names
were being evaluated.

These changes resolve both of the issues by treating all `HostBinding` instance as if they're
reading the property from `this`. E.g. the `@HostBinding('class.foo') public true = 1;` from above
is now being treated as `host: {'[class.foo]': 'this.true'}` which further down the pipeline becomes
`classProp('foo', ctx.true)`. This doesn't have any payload size implications for existing code,
because we've always been prefixing implicit property reads with `ctx.`. If the property doesn't
have an identifier that can be read using dotted access, we convert it to a quoted one (e.g.
`classProp('foo', ctx['is-foo']))`.

Fixes angular#40220.
Fixes angular#40230.
crisbeto added a commit to crisbeto/angular that referenced this issue Dec 22, 2020
@crisbeto
fix(compiler): incorrectly interpreting some HostBinding names
2b1812a 
Currently when analyzing the metadata of a directive, we bundle together the bindings from `host`
and the `HostBinding` and `HostListener` together. This can become a problem later on in the
compilation pipeline, because we try to evaluate the value of the binding, causing something like
`@HostBinding('class.foo') public true = 1;` to be treated the same as
`host: {'[class.foo]': 'true'}`.

While looking into the issue, I noticed another one that is closely related: we weren't treating
quoted property names correctly. E.g. `@HostBinding('class.foo') public "foo-bar" = 1;` was being
interpreted as `classProp('foo', ctx.foo - ctx.bar)` due to the same issue where property names
were being evaluated.

These changes resolve both of the issues by treating all `HostBinding` instance as if they're
reading the property from `this`. E.g. the `@HostBinding('class.foo') public true = 1;` from above
is now being treated as `host: {'[class.foo]': 'this.true'}` which further down the pipeline becomes
`classProp('foo', ctx.true)`. This doesn't have any payload size implications for existing code,
because we've always been prefixing implicit property reads with `ctx.`. If the property doesn't
have an identifier that can be read using dotted access, we convert it to a quoted one (e.g.
`classProp('foo', ctx['is-foo']))`.

Fixes angular#40220.
Fixes angular#40230.
crisbeto added a commit to crisbeto/angular that referenced this issue Dec 22, 2020
@crisbeto
fix(compiler): incorrectly interpreting some HostBinding names
ff5e46f 
Currently when analyzing the metadata of a directive, we bundle together the bindings from `host`
and the `HostBinding` and `HostListener` together. This can become a problem later on in the
compilation pipeline, because we try to evaluate the value of the binding, causing something like
`@HostBinding('class.foo') public true = 1;` to be treated the same as
`host: {'[class.foo]': 'true'}`.

While looking into the issue, I noticed another one that is closely related: we weren't treating
quoted property names correctly. E.g. `@HostBinding('class.foo') public "foo-bar" = 1;` was being
interpreted as `classProp('foo', ctx.foo - ctx.bar)` due to the same issue where property names
were being evaluated.

These changes resolve both of the issues by treating all `HostBinding` instance as if they're
reading the property from `this`. E.g. the `@HostBinding('class.foo') public true = 1;` from above
is now being treated as `host: {'[class.foo]': 'this.true'}` which further down the pipeline becomes
`classProp('foo', ctx.true)`. This doesn't have any payload size implications for existing code,
because we've always been prefixing implicit property reads with `ctx.`. If the property doesn't
have an identifier that can be read using dotted access, we convert it to a quoted one (e.g.
`classProp('foo', ctx['is-foo']))`.

Fixes angular#40220.
Fixes angular#40230.
@crisbeto crisbeto self-assigned this Dec 22, 2020
@crisbeto crisbeto added area: compiler Issues related to `ngc`, Angular's template compiler area: core Issues related to the framework runtime P4 A relatively minor issue that is not relevant to core functions state: has PR labels Dec 22, 2020
@ngbot ngbot bot modified the milestone: Backlog Dec 22, 2020
@crisbeto crisbeto changed the title Host bindings generated invalid code for quoted property names with dashes Host bindings generate invalid code for quoted property names with dashes Dec 22, 2020
crisbeto added a commit to crisbeto/angular that referenced this issue Dec 29, 2020
@crisbeto
fix(compiler): incorrectly interpreting some HostBinding names
b4ebec6 
Currently when analyzing the metadata of a directive, we bundle together the bindings from `host`
and the `HostBinding` and `HostListener` together. This can become a problem later on in the
compilation pipeline, because we try to evaluate the value of the binding, causing something like
`@HostBinding('class.foo') public true = 1;` to be treated the same as
`host: {'[class.foo]': 'true'}`.

While looking into the issue, I noticed another one that is closely related: we weren't treating
quoted property names correctly. E.g. `@HostBinding('class.foo') public "foo-bar" = 1;` was being
interpreted as `classProp('foo', ctx.foo - ctx.bar)` due to the same issue where property names
were being evaluated.

These changes resolve both of the issues by treating all `HostBinding` instance as if they're
reading the property from `this`. E.g. the `@HostBinding('class.foo') public true = 1;` from above
is now being treated as `host: {'[class.foo]': 'this.true'}` which further down the pipeline becomes
`classProp('foo', ctx.true)`. This doesn't have any payload size implications for existing code,
because we've always been prefixing implicit property reads with `ctx.`. If the property doesn't
have an identifier that can be read using dotted access, we convert it to a quoted one (e.g.
`classProp('foo', ctx['is-foo']))`.

Fixes angular#40220.
Fixes angular#40230.
crisbeto added a commit to crisbeto/angular that referenced this issue Dec 31, 2020
@crisbeto
fix(compiler): incorrectly interpreting some HostBinding names
f7bdfce 
Currently when analyzing the metadata of a directive, we bundle together the bindings from `host`
and the `HostBinding` and `HostListener` together. This can become a problem later on in the
compilation pipeline, because we try to evaluate the value of the binding, causing something like
`@HostBinding('class.foo') public true = 1;` to be treated the same as
`host: {'[class.foo]': 'true'}`.

While looking into the issue, I noticed another one that is closely related: we weren't treating
quoted property names correctly. E.g. `@HostBinding('class.foo') public "foo-bar" = 1;` was being
interpreted as `classProp('foo', ctx.foo - ctx.bar)` due to the same issue where property names
were being evaluated.

These changes resolve both of the issues by treating all `HostBinding` instance as if they're
reading the property from `this`. E.g. the `@HostBinding('class.foo') public true = 1;` from above
is now being treated as `host: {'[class.foo]': 'this.true'}` which further down the pipeline becomes
`classProp('foo', ctx.true)`. This doesn't have any payload size implications for existing code,
because we've always been prefixing implicit property reads with `ctx.`. If the property doesn't
have an identifier that can be read using dotted access, we convert it to a quoted one (e.g.
`classProp('foo', ctx['is-foo']))`.

Fixes angular#40220.
Fixes angular#40230.
Fixes angular#18698.
crisbeto added a commit to crisbeto/angular that referenced this issue Jan 6, 2021
@crisbeto
fix(compiler): incorrectly interpreting some HostBinding names
a49b361 
Currently when analyzing the metadata of a directive, we bundle together the bindings from `host`
and the `HostBinding` and `HostListener` together. This can become a problem later on in the
compilation pipeline, because we try to evaluate the value of the binding, causing something like
`@HostBinding('class.foo') public true = 1;` to be treated the same as
`host: {'[class.foo]': 'true'}`.

While looking into the issue, I noticed another one that is closely related: we weren't treating
quoted property names correctly. E.g. `@HostBinding('class.foo') public "foo-bar" = 1;` was being
interpreted as `classProp('foo', ctx.foo - ctx.bar)` due to the same issue where property names
were being evaluated.

These changes resolve both of the issues by treating all `HostBinding` instance as if they're
reading the property from `this`. E.g. the `@HostBinding('class.foo') public true = 1;` from above
is now being treated as `host: {'[class.foo]': 'this.true'}` which further down the pipeline becomes
`classProp('foo', ctx.true)`. This doesn't have any payload size implications for existing code,
because we've always been prefixing implicit property reads with `ctx.`. If the property doesn't
have an identifier that can be read using dotted access, we convert it to a quoted one (e.g.
`classProp('foo', ctx['is-foo']))`.

Fixes angular#40220.
Fixes angular#40230.
Fixes angular#18698.
@atscott atscott closed this as completed in 1045465 Jan 7, 2021
@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 Feb 7, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees

@crisbeto crisbeto

Labels
area: compiler Issues related to `ngc`, Angular's template compiler area: core Issues related to the framework runtime P4 A relatively minor issue that is not relevant to core functions state: confirmed state: has PR
Projects
None yet
Milestone
Backlog
Development

Successfully merging a pull request may close this issue.

fix(compiler): incorrectly interpreting some HostBinding names crisbeto/angular
1 participant
@crisbeto