Skip to content
This repository was archived by the owner on Sep 5, 2024. It is now read-only.

Conversation

@oliversalzburg
Copy link
Contributor

@oliversalzburg oliversalzburg commented Feb 14, 2020

PR Checklist

Please check that your PR fulfills the following requirements:

  • The commit message follows our guidelines
  • Tests for the changes have been added or this is not a bug fix / enhancement
  • Docs have been added, updated, or were not required

PR Type

What kind of change does this PR introduce?

[X] Bugfix
[X] Enhancement
[ ] Documentation content changes
[ ] Code style update (formatting, local variables)
[ ] Refactoring (no functional changes, no api changes)
[ ] Build related changes
[ ] CI related changes
[ ] Infrastructure changes
[ ] Other... Please describe:

What is the current behavior?

Even though the $onInit lifecycle hook is fully supported, the $onDestroy hook isn't. This is unexpected for developers.

What is the new behavior?

This change introduces a listener for the $destroy event on the linked scope. See https://docs.angularjs.org/api/ng/type/$rootScope.Scope#event-$destroy

This implementation closely resembles the AngularJS internal implementation of the lifecycle hook invokation behavior: https://github.com/angular/angular.js/blob/2b28c540ad7ebf4a9c3a6f108a9cb5b673d3712d/src/ng/compile.js#L3298-L3302

Fixes #11847

Does this PR introduce a breaking change?

[ ] Yes
[X] No

@googlebot googlebot added the cla: yes PR author has signed Google's CLA: https://opensource.google.com/docs/cla/ label Feb 14, 2020
@Splaktar Splaktar self-requested a review March 5, 2020 09:25
@Splaktar Splaktar self-assigned this Mar 5, 2020
Copy link
Contributor

@Splaktar Splaktar left a comment

Choose a reason for hiding this comment

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

I'm happy with these changes, but we need some unit tests to cover this functionality.

@Splaktar Splaktar added this to the 1.1.23 milestone Mar 7, 2020
@Splaktar Splaktar added in progress Mainly for in progress PRs, but may be used for issues that require multiple PRs needs: unit tests This PR needs unit tests to cover the changes being proposed P3: important Important issues that really should be fixed when possible. labels Mar 7, 2020
@Splaktar Splaktar assigned oliversalzburg and unassigned Splaktar Mar 7, 2020
@oliversalzburg oliversalzburg requested a review from Splaktar March 13, 2020 11:46
Copy link
Contributor

@Splaktar Splaktar left a comment

Choose a reason for hiding this comment

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

LGTM

@Splaktar Splaktar added pr: merge ready This PR is ready for a caretaker to review and removed in progress Mainly for in progress PRs, but may be used for issues that require multiple PRs needs: unit tests This PR needs unit tests to cover the changes being proposed labels Apr 27, 2020
@Splaktar Splaktar requested a review from jelbourn April 27, 2020 22:53
Even though the $onInit lifecycle hook is fully supported, the
$onDestroy hook isn't. This is unexpected for developers.

This change introduces a listener for the $destroy event on the linked
scope. See https://docs.angularjs.org/api/ng/type/$rootScope.Scope#event-$destroy

This implementation closely resembles the AngularJS internal
implementation of the lifecycle hook invokation behavior:
https://github.com/angular/angular.js/blob/2b28c540ad7ebf4a9c3a6f108a9cb5b673d3712d/src/ng/compile.js#L3298-L3302

Fixes #11847
Copy link
Member

@jelbourn jelbourn left a comment

Choose a reason for hiding this comment

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

LGTM

@Splaktar Splaktar added the pr: lgtm This PR has been approved by the reviewer label Apr 30, 2020
@Splaktar Splaktar merged commit 8bb1d98 into angular:master Apr 30, 2020
@oliversalzburg oliversalzburg deleted the fix/panel-on-destroy branch May 7, 2020 10:16
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

cla: yes PR author has signed Google's CLA: https://opensource.google.com/docs/cla/ P3: important Important issues that really should be fixed when possible. pr: lgtm This PR has been approved by the reviewer pr: merge ready This PR is ready for a caretaker to review type: enhancement

Projects

None yet

Development

Successfully merging this pull request may close these issues.

mdCompiler: $onDestroy lifecycle hook never called

4 participants