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’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

HostBinding supports an undocumented magic property name(?) #40220

Closed
Airblader opened this issue Dec 21, 2020 · 2 comments
Closed

HostBinding supports an undocumented magic property name(?) #40220

Airblader opened this issue Dec 21, 2020 · 2 comments
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

@Airblader
Copy link
Contributor

Airblader commented Dec 21, 2020 •
edited

🐞 bug report

Affected Package

The issue is caused by package @angular/core (?)

Is this a regression?

No.

Description

Let's look at the following component:

@Component({ … })
public class TestComponent {
  @HostBinding('class.a') true;
  @HostBinding('class.b') false;
  @HostBinding('class.c') other;
}

This component ends up with the class a on its host, but neither one of the other two. The latter part is what I would expect, given that all three lines define a class member implicitly typed any initialized as undefined, a falsy value. It appears, however, that the name "true" is somehow treated like a magic value.

This is surprising, and undocumented as far as I can tell. In fact, in modern projects this would be an error due to noImplicitAny as well as strictPropertyInitialization.

This could of course also be some ES quirk that I'm not familiar with, but at least this behaves as expected (in plain JS):

class Test { true; }
const test = new Test();
console.log(test.true); // undefined

As far as I can tell there's no test for this behavior in Angular, so I don't know if this works accidentally or whether in a time before noImplicitAny this was considered a feature. The documentation all the way back to v2 also doesn't mention this.

🔬 Minimal Reproduction

https://stackblitz.com/edit/angular-ivy-5dzgsw?file=src/app/hello.component.ts

🌍 Your Environment

Angular Version:

Given that there are StackOverflow posts dating back years I assume this has existed for a long, long time.


    "@angular/common": "^10.0.9",
    "@angular/compiler": "^10.0.9",
    "@angular/core": "^10.0.9",
@Airblader
Copy link
Contributor Author

Ultimately my request here is to give a statement on why this happens (feature? coincidence?), whether this should be kept (I vote no given that it fails with noImplicitAny anyway), and failing that it should at least be documented.

@josephperrott josephperrott added the area: core Issues related to the framework runtime label Dec 21, 2020
@ngbot ngbot bot added this to the needsTriage milestone Dec 21, 2020
@crisbeto crisbeto self-assigned this Dec 22, 2020
@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
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 added area: compiler Issues related to `ngc`, Angular's template compiler P4 A relatively minor issue that is not relevant to core functions state: has PR labels Dec 22, 2020
@ngbot ngbot bot modified the milestones: needsTriage, Backlog 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
3 participants
@Airblader @crisbeto @josephperrott