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(elements): fire custom element output events during component initialization #36161

Closed

Conversation

gkalpak
Copy link
Member

@gkalpak gkalpak commented Mar 20, 2020

This PR sits on top of #36114 (to avoid conflicts), so only the last two commits are new.

Previously, event listeners for component output events attached on an Angular custom element before inserting it into the DOM (i.e. before instantiating the underlying component) didn't fire for events emitted during initialization lifecycle hooks, such as ngAfterContentInit, ngAfterViewInit, ngOnChanges (initial call) and ngOnInit.
The reason was that that NgElementImpl subscribed to events after calling ngElementStrategy#connect(), which is where the initial change detection takes place (running the initialization lifecycle hooks).

This commit fixes this by:

  1. Ensuring ComponentNgElementStrategy#events is defined and available for subscribing to, even before instantiating the component.
  2. Ensuring NgElementImpl subscribes to NgElementStrategy#events before calling NgElementStrategy#connect() (which initializes the component instance).

Jira issue: FW-2010

Fixes #36141.

For future reference:
This PR increases the main bundle by ~190B (currently 453658B --> 453847B). This is caused by the newly added === null/!== null checks, the extra this.componentRefs passed as arguments to methods and the eventEmitters and events initialization 🤦‍♂

@gkalpak gkalpak added type: bug/fix action: merge The PR is ready for merge by the caretaker target: patch This PR is targeted for the next patch release area: elements Issues related to Angular Elements labels Mar 20, 2020
@ngbot ngbot bot modified the milestone: needsTriage Mar 20, 2020
@mary-poppins
Copy link

You can preview ace1bd3 at https://pr36161-ace1bd3.ngbuilds.io/.

@mhevery mhevery removed the action: merge The PR is ready for merge by the caretaker label Mar 20, 2020
@gkalpak gkalpak force-pushed the fix-elements-outputs-during-init branch from ace1bd3 to cb2871e Compare March 20, 2020 17:43
@mary-poppins
Copy link

You can preview cb2871e at https://pr36161-cb2871e.ngbuilds.io/.

@gkalpak gkalpak force-pushed the fix-elements-outputs-during-init branch from cb2871e to c816e62 Compare March 20, 2020 20:09
@@ -12,7 +12,7 @@
"master": {
"uncompressed": {
"runtime-es2015": 2987,
"main-es2015": 451469,
"main-es2015": 452031,
Copy link
Member Author

Choose a reason for hiding this comment

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

This ~190B increase (for the commits that are new in this PR) is caused by the newly added === null/!== null checks, the extra this.componentRefs passed as arguments to methods and the eventEmitters and events initialization 🤦‍♂

@mary-poppins
Copy link

You can preview c816e62 at https://pr36161-c816e62.ngbuilds.io/.

@gkalpak gkalpak marked this pull request as ready for review March 20, 2020 20:32
@gkalpak gkalpak added the action: merge The PR is ready for merge by the caretaker label Mar 20, 2020
@gkalpak gkalpak added state: blocked action: review The PR is still awaiting reviews from at least one requested reviewer and removed action: merge The PR is ready for merge by the caretaker labels Mar 20, 2020
@gkalpak
Copy link
Member Author

gkalpak commented Mar 20, 2020

Marking this as blocked on #36114, because that had to go in first.
(But this is still good for review - the last two commits, which are new.)

@gkalpak gkalpak removed the request for review from petebacondarwin March 20, 2020 20:39
Copy link
Contributor

@andrewseguin andrewseguin left a comment

Choose a reason for hiding this comment

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

LGTM

@mary-poppins
Copy link

You can preview f70b5ed at https://pr36161-f70b5ed.ngbuilds.io/.

@gkalpak gkalpak force-pushed the fix-elements-outputs-during-init branch from f70b5ed to 3b75c64 Compare May 1, 2020 23:22
@gkalpak gkalpak added action: merge The PR is ready for merge by the caretaker target: major This PR is targeted for the next major release and removed target: patch This PR is targeted for the next patch release labels May 21, 2020
@gkalpak
Copy link
Member Author

gkalpak commented May 21, 2020

This requires a payload size bump for ViewEngine on 9.1.x.
Let's merge this into master and 10.0.x (RC) only. I have prepared #37231 for 9.1.x.

@gkalpak gkalpak added target: patch This PR is targeted for the next patch release and removed target: major This PR is targeted for the next major release labels May 22, 2020
@gkalpak gkalpak added the action: presubmit The PR is in need of a google3 presubmit label Jun 5, 2020
…y` type-casts

This commit removes some unnecessary non-null assertions (`!`) and
`as any` type-casts from the `elements` package.
…tialization

Previously, event listeners for component output events attached on an
Angular custom element before inserting it into the DOM (i.e. before
instantiating the underlying component) didn't fire for events emitted
during initialization lifecycle hooks, such as `ngAfterContentInit`,
`ngAfterViewInit`, `ngOnChanges` (initial call) and `ngOnInit`.
The reason was that that `NgElementImpl` [subscribed to events][1]
_after_ calling [ngElementStrategy#connect()][2], which is where the
[initial change detection][3] takes place (running the initialization
lifecycle hooks).

This commit fixes this by:
1. Ensuring `ComponentNgElementStrategy#events` is defined and available
   for subscribing to, even before instantiating the component.
2. Ensuring `NgElementImpl` subscribes to `NgElementStrategy#events`
   before calling `NgElementStrategy#connect()` (which initializes the
   component instance).

Jira issue: [FW-2010](https://angular-team.atlassian.net/browse/FW-2010)

[1]: https://github.com/angular/angular/blob/c0143cb2abdd172de1b95fd1d2c4cfc738640e28/packages/elements/src/create-custom-element.ts#L167-L170
[2]: https://github.com/angular/angular/blob/c0143cb2abdd172de1b95fd1d2c4cfc738640e28/packages/elements/src/create-custom-element.ts#L164
[3]: https://github.com/angular/angular/blob/c0143cb2abdd172de1b95fd1d2c4cfc738640e28/packages/elements/src/component-factory-strategy.ts#L158

Fixes angular#36141
@gkalpak gkalpak force-pushed the fix-elements-outputs-during-init branch from 68d90e3 to 6a35a52 Compare June 5, 2020 17:14
@mary-poppins
Copy link

You can preview 6a35a52 at https://pr36161-6a35a52.ngbuilds.io/.

atscott pushed a commit that referenced this pull request Jun 5, 2020
…y` type-casts (#36161)

This commit removes some unnecessary non-null assertions (`!`) and
`as any` type-casts from the `elements` package.

PR Close #36161
atscott pushed a commit that referenced this pull request Jun 5, 2020
…tialization (#36161)

Previously, event listeners for component output events attached on an
Angular custom element before inserting it into the DOM (i.e. before
instantiating the underlying component) didn't fire for events emitted
during initialization lifecycle hooks, such as `ngAfterContentInit`,
`ngAfterViewInit`, `ngOnChanges` (initial call) and `ngOnInit`.
The reason was that that `NgElementImpl` [subscribed to events][1]
_after_ calling [ngElementStrategy#connect()][2], which is where the
[initial change detection][3] takes place (running the initialization
lifecycle hooks).

This commit fixes this by:
1. Ensuring `ComponentNgElementStrategy#events` is defined and available
   for subscribing to, even before instantiating the component.
2. Ensuring `NgElementImpl` subscribes to `NgElementStrategy#events`
   before calling `NgElementStrategy#connect()` (which initializes the
   component instance).

Jira issue: [FW-2010](https://angular-team.atlassian.net/browse/FW-2010)

[1]: https://github.com/angular/angular/blob/c0143cb2abdd172de1b95fd1d2c4cfc738640e28/packages/elements/src/create-custom-element.ts#L167-L170
[2]: https://github.com/angular/angular/blob/c0143cb2abdd172de1b95fd1d2c4cfc738640e28/packages/elements/src/create-custom-element.ts#L164
[3]: https://github.com/angular/angular/blob/c0143cb2abdd172de1b95fd1d2c4cfc738640e28/packages/elements/src/component-factory-strategy.ts#L158

Fixes #36141

PR Close #36161
@atscott atscott closed this in 569d1ef Jun 5, 2020
atscott pushed a commit that referenced this pull request Jun 5, 2020
…tialization (#36161)

Previously, event listeners for component output events attached on an
Angular custom element before inserting it into the DOM (i.e. before
instantiating the underlying component) didn't fire for events emitted
during initialization lifecycle hooks, such as `ngAfterContentInit`,
`ngAfterViewInit`, `ngOnChanges` (initial call) and `ngOnInit`.
The reason was that that `NgElementImpl` [subscribed to events][1]
_after_ calling [ngElementStrategy#connect()][2], which is where the
[initial change detection][3] takes place (running the initialization
lifecycle hooks).

This commit fixes this by:
1. Ensuring `ComponentNgElementStrategy#events` is defined and available
   for subscribing to, even before instantiating the component.
2. Ensuring `NgElementImpl` subscribes to `NgElementStrategy#events`
   before calling `NgElementStrategy#connect()` (which initializes the
   component instance).

Jira issue: [FW-2010](https://angular-team.atlassian.net/browse/FW-2010)

[1]: https://github.com/angular/angular/blob/c0143cb2abdd172de1b95fd1d2c4cfc738640e28/packages/elements/src/create-custom-element.ts#L167-L170
[2]: https://github.com/angular/angular/blob/c0143cb2abdd172de1b95fd1d2c4cfc738640e28/packages/elements/src/create-custom-element.ts#L164
[3]: https://github.com/angular/angular/blob/c0143cb2abdd172de1b95fd1d2c4cfc738640e28/packages/elements/src/component-factory-strategy.ts#L158

Fixes #36141

PR Close #36161
@gkalpak gkalpak deleted the fix-elements-outputs-during-init branch June 5, 2020 18:59
atscott added a commit to atscott/angular that referenced this pull request Jun 10, 2020
…nent initialization (angular#36161)"

This reverts commit e9bff5f. Failures
were detected in Google tests due to this commit
atscott added a commit to atscott/angular that referenced this pull request Jun 10, 2020
…nent initialization (angular#36161)"

This reverts commit e9bff5f. Failures
were detected by Google tests after due to this commit.
atscott added a commit that referenced this pull request Jun 10, 2020
…nent initialization (#36161)" (#37524)

This reverts commit e9bff5f. Failures
were detected in Google tests due to this commit

PR Close #37524
atscott added a commit that referenced this pull request Jun 10, 2020
…nent initialization (#36161)" (#37524)

This reverts commit e9bff5f. Failures
were detected in Google tests due to this commit

PR Close #37524
ngwattcos pushed a commit to ngwattcos/angular that referenced this pull request Jun 25, 2020
…y` type-casts (angular#36161)

This commit removes some unnecessary non-null assertions (`!`) and
`as any` type-casts from the `elements` package.

PR Close angular#36161
ngwattcos pushed a commit to ngwattcos/angular that referenced this pull request Jun 25, 2020
…tialization (angular#36161)

Previously, event listeners for component output events attached on an
Angular custom element before inserting it into the DOM (i.e. before
instantiating the underlying component) didn't fire for events emitted
during initialization lifecycle hooks, such as `ngAfterContentInit`,
`ngAfterViewInit`, `ngOnChanges` (initial call) and `ngOnInit`.
The reason was that that `NgElementImpl` [subscribed to events][1]
_after_ calling [ngElementStrategy#connect()][2], which is where the
[initial change detection][3] takes place (running the initialization
lifecycle hooks).

This commit fixes this by:
1. Ensuring `ComponentNgElementStrategy#events` is defined and available
   for subscribing to, even before instantiating the component.
2. Ensuring `NgElementImpl` subscribes to `NgElementStrategy#events`
   before calling `NgElementStrategy#connect()` (which initializes the
   component instance).

Jira issue: [FW-2010](https://angular-team.atlassian.net/browse/FW-2010)

[1]: https://github.com/angular/angular/blob/c0143cb2abdd172de1b95fd1d2c4cfc738640e28/packages/elements/src/create-custom-element.ts#L167-L170
[2]: https://github.com/angular/angular/blob/c0143cb2abdd172de1b95fd1d2c4cfc738640e28/packages/elements/src/create-custom-element.ts#L164
[3]: https://github.com/angular/angular/blob/c0143cb2abdd172de1b95fd1d2c4cfc738640e28/packages/elements/src/component-factory-strategy.ts#L158

Fixes angular#36141

PR Close angular#36161
ngwattcos pushed a commit to ngwattcos/angular that referenced this pull request Jun 25, 2020
…nent initialization (angular#36161)" (angular#37524)

This reverts commit e9bff5f. Failures
were detected in Google tests due to this commit

PR Close angular#37524
@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 Jul 6, 2020
profanis pushed a commit to profanis/angular that referenced this pull request Sep 5, 2020
…y` type-casts (angular#36161)

This commit removes some unnecessary non-null assertions (`!`) and
`as any` type-casts from the `elements` package.

PR Close angular#36161
profanis pushed a commit to profanis/angular that referenced this pull request Sep 5, 2020
…tialization (angular#36161)

Previously, event listeners for component output events attached on an
Angular custom element before inserting it into the DOM (i.e. before
instantiating the underlying component) didn't fire for events emitted
during initialization lifecycle hooks, such as `ngAfterContentInit`,
`ngAfterViewInit`, `ngOnChanges` (initial call) and `ngOnInit`.
The reason was that that `NgElementImpl` [subscribed to events][1]
_after_ calling [ngElementStrategy#connect()][2], which is where the
[initial change detection][3] takes place (running the initialization
lifecycle hooks).

This commit fixes this by:
1. Ensuring `ComponentNgElementStrategy#events` is defined and available
   for subscribing to, even before instantiating the component.
2. Ensuring `NgElementImpl` subscribes to `NgElementStrategy#events`
   before calling `NgElementStrategy#connect()` (which initializes the
   component instance).

Jira issue: [FW-2010](https://angular-team.atlassian.net/browse/FW-2010)

[1]: https://github.com/angular/angular/blob/c0143cb2abdd172de1b95fd1d2c4cfc738640e28/packages/elements/src/create-custom-element.ts#L167-L170
[2]: https://github.com/angular/angular/blob/c0143cb2abdd172de1b95fd1d2c4cfc738640e28/packages/elements/src/create-custom-element.ts#L164
[3]: https://github.com/angular/angular/blob/c0143cb2abdd172de1b95fd1d2c4cfc738640e28/packages/elements/src/component-factory-strategy.ts#L158

Fixes angular#36141

PR Close angular#36161
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: elements Issues related to Angular Elements cla: yes target: patch This PR is targeted for the next patch release type: bug/fix
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Custom Element output events not firing during initialization lifecycle hooks
5 participants