X Tutup
The Wayback Machine - https://web.archive.org/web/20211020194522/https://github.com/angular/angular/pull/39323
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): preserve this.$event and this.$any accesses in expressions #39323

Closed
wants to merge 3 commits into from

Conversation

@crisbeto
Copy link
Member

@crisbeto crisbeto commented Oct 18, 2020

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 Oct 18, 2020
@crisbeto crisbeto marked this pull request as ready for review Oct 18, 2020
@ngbot ngbot bot added this to the needsTriage milestone Oct 18, 2020
@ngbot ngbot bot added this to the needsTriage milestone Oct 18, 2020
@pullapprove pullapprove bot requested review from AndrewKushnir and ayazhafiz Oct 18, 2020
@AndrewKushnir AndrewKushnir requested review from alxhub and removed request for AndrewKushnir Oct 18, 2020
alxhub
alxhub approved these changes Oct 29, 2020
packages/compiler/src/expression_parser/ast.ts Outdated Show resolved Hide resolved
packages/compiler/src/render3/view/template.ts Outdated Show resolved Hide resolved
packages/core/test/acceptance/integration_spec.ts Outdated Show resolved Hide resolved
crisbeto added 2 commits Oct 29, 2020
…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.
Copy link
Contributor

@ayazhafiz ayazhafiz left a comment

Nice change, LGTM for language service

@josephperrott
Copy link
Member

@josephperrott josephperrott commented Oct 29, 2020

@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.

@crisbeto
Copy link
Member Author

@crisbeto crisbeto commented Oct 30, 2020

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

@josephperrott
Copy link
Member

@josephperrott josephperrott commented Oct 30, 2020

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

josephperrott added a commit that referenced this issue 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

@angular-automatic-lock-bot angular-automatic-lock-bot bot commented Nov 30, 2020

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.
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

4 participants
X Tutup