Sitelet https://github.com/angular/angular/pull/39323
Skip to content

fix(compiler): preserve this.$event and this.$any accesses in expressions - #39323

Closed
crisbeto wants to merge 3 commits into
angular:masterfrom
crisbeto:30278/$event-access
Closed

crisbeto wants to merge 3 commits into
angular:masterfrom
crisbeto:30278/$event-access

Conversation

@crisbeto

@crisbeto crisbeto commented Oct 18, 2020 •

Copy link
Copy Markdown
Member

Currently expressions like $event.foo() and this.$event.foo(), as well as $any(foo) and this.$any(foo), are treated as the same expression by the compiler, because this is considered the same implicit receiver as when the receiver is omitted. This introduces the following issues:

  1. Any time something called $any is used, it'll be stripped away, leaving only the first parameter.
  2. If something called $event is used anywhere in a template, it'll be preserved as $event, rather than being rewritten to ctx.$event, causing the value to undefined at runtime. This applies to listener, property and text bindings.

These changes resolve the first issue and part of the second one by preserving anything that is accessed through this, even if it's one of the "special" ones like $any or $event. Furthermore, these changes only expose the $event global variable inside event listeners, whereas previously it was available everywhere.

Fixes #30278.

@google-cla google-cla Bot added the cla: yes label Oct 18, 2020
@crisbeto
crisbeto force-pushed the 30278/$event-access branch from 0135583 to 326ea9f Compare October 18, 2020 15:41
@crisbeto crisbeto added action: review The PR is still awaiting reviews from at least one requested reviewer area: compiler Issues related to `ngc`, Angular's template compiler target: patch This PR is targeted for the next patch release type: bug/fix labels Oct 18, 2020
@crisbeto
crisbeto marked this pull request as ready for review October 18, 2020 17:23
@ngbot ngbot Bot modified the milestone: needsTriage Oct 18, 2020
@AndrewKushnir
AndrewKushnir requested review from alxhub and removed request for AndrewKushnir October 18, 2020 17:55
Comment thread packages/compiler/src/expression_parser/ast.ts Outdated
Comment thread packages/compiler/src/render3/view/template.ts Outdated
Comment thread packages/core/test/acceptance/integration_spec.ts Outdated
…ions

Currently expressions `$event.foo()` and `this.$event.foo()`, as well as `$any(foo)` and
`this.$any(foo)`, are treated as the same expression by the compiler, because `this` is considered
the same implicit receiver as when the receiver is omitted. This introduces the following issues:

1. Any time something called `$any` is used, it'll be stripped away, leaving only the first parameter.
2. If something called `$event` is used anywhere in a template, it'll be preserved as `$event`,
rather than being rewritten to `ctx.$event`, causing the value to undefined at runtime. This
applies to listener, property and text bindings.

These changes resolve the first issue and part of the second one by preserving anything that
is accessed through `this`, even if it's one of the "special" ones like `$any` or `$event`.
Furthermore, these changes only expose the `$event` global variable inside event listeners,
whereas previously it was available everywhere.

Fixes angular#30278.
@crisbeto
crisbeto force-pushed the 30278/$event-access branch from 326ea9f to 72e9cae Compare October 29, 2020 20:42
@crisbeto crisbeto added action: merge The PR is ready for merge by the caretaker merge: caretaker note Alert the caretaker performing the merge to check the PR for an out of normal action needed or note action: presubmit The PR is in need of a google3 presubmit and removed action: review The PR is still awaiting reviews from at least one requested reviewer labels Oct 29, 2020

@ayazhafiz ayazhafiz left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice change, LGTM for language service

@josephperrott

Copy link
Copy Markdown
Member

@crisbeto Looks like this breaks codelyzer, so it doesn't land cleanly.

While its marked as a fix here, it does modify the type of AstVisitor to include a new method. So everything extending it fails to build.

@josephperrott josephperrott removed action: merge The PR is ready for merge by the caretaker merge: caretaker note Alert the caretaker performing the merge to check the PR for an out of normal action needed or note labels Oct 29, 2020
@crisbeto

Copy link
Copy Markdown
Member Author

@josephperrott I've made the new method optional. Can we give it another try?

@crisbeto crisbeto added action: merge The PR is ready for merge by the caretaker merge: caretaker note Alert the caretaker performing the merge to check the PR for an out of normal action needed or note labels Oct 30, 2020
@josephperrott josephperrott added target: rc This PR is targeted for the next release-candidate and removed target: patch This PR is targeted for the next patch release labels Oct 30, 2020
@josephperrott

Copy link
Copy Markdown
Member

Updating to target: rc as discussed offline due to the change not cleanly landing in 10.2.x

@josephperrott josephperrott removed the merge: caretaker note Alert the caretaker performing the merge to check the PR for an out of normal action needed or note label Oct 30, 2020
josephperrott pushed a commit that referenced this pull request Oct 30, 2020
…ions (#39323)

Currently expressions `$event.foo()` and `this.$event.foo()`, as well as `$any(foo)` and
`this.$any(foo)`, are treated as the same expression by the compiler, because `this` is considered
the same implicit receiver as when the receiver is omitted. This introduces the following issues:

1. Any time something called `$any` is used, it'll be stripped away, leaving only the first parameter.
2. If something called `$event` is used anywhere in a template, it'll be preserved as `$event`,
rather than being rewritten to `ctx.$event`, causing the value to undefined at runtime. This
applies to listener, property and text bindings.

These changes resolve the first issue and part of the second one by preserving anything that
is accessed through `this`, even if it's one of the "special" ones like `$any` or `$event`.
Furthermore, these changes only expose the `$event` global variable inside event listeners,
whereas previously it was available everywhere.

Fixes #30278.

PR Close #39323
@angular-automatic-lock-bot

Copy link
Copy Markdown

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 Nov 30, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

action: merge The PR is ready for merge by the caretaker action: presubmit The PR is in need of a google3 presubmit area: compiler Issues related to `ngc`, Angular's template compiler cla: yes target: rc This PR is targeted for the next release-candidate type: bug/fix

Projects

None yet

Development

Successfully merging this pull request may close these issues.

bug(compiler): this.$event and this.$any() should not be allowed in template

4 participants