fix(animations): cleanup DOM elements when the root view is removed #45143

arturovt · 2022-02-19T18:36:03Z

PR Checklist

The commit message follows our guidelines: https://github.com/angular/angular/blob/master/CONTRIBUTING.md#commit
Tests for the changes have been added (for bug fixes / features)

PR Type

Bugfix

What is the current behavior?

Issue Number: #45108

JoostK · 2022-02-23T20:56:47Z

Judging by the commit description, would this issue be resolved if InjectableAnimationEngine were to inject ApplicationRef to force dependency ordering? I feel very uneasy to spawn asynchronous cleanup actions from within ngOnDestroy, as this kind of violates lifecycle expectations where the class is in a destroyed state and shouldn't be interacted with anymore.

arturovt · 2022-02-23T21:00:51Z

Judging by the commit description, would this issue be resolved if InjectableAnimationEngine were to inject ApplicationRef to force dependency ordering? I feel very uneasy to spawn asynchronous cleanup actions from within ngOnDestroy, as this kind of violates lifecycle expectations where the class is in a destroyed state and shouldn't be interacted with anymore.

Yeah, that sounds more reasonable than my implementation.

arturovt · 2022-02-24T02:50:51Z

@JoostK I've tried injecting ApplicationRef into animation engine and I'm getting Circular dependency in DI detected for ApplicationRef. This is because ApplicationRef depends on ApplicationInitStatus, the ApplicationInitStatus injects APP_INITIALIZER, which may have deps: [...] that depend on AnimationEngine. Any ideas?

JoostK · 2022-02-24T14:06:42Z

Ah, I was afraid that might happen. One possibility could be to remove ApplicationInitStatus from the constructor and retrieve it lazily using the Injector, which is already stored in ApplicationRef in the _injector field.

arturovt · 2022-03-10T22:31:56Z

@JoostK I've made updates, the CI is green. The only failing job seems to be unrelated to the change (not sure why it's failing).

JoostK

Thanks for working on this! I have added @jessicajaniuk as reviewer as I'm not a code owner for these parts of the FW, but I did leave some suggestions to clarify the tests.

packages/platform-browser/animations/test/animation_renderer_spec.ts

jessicajaniuk

LGTM 🍪

This seems good. I've left one minor suggestion and I'm also curious about your answers to @JoostK's questions.

packages/platform-browser/animations/src/providers.ts

JoostK · 2022-03-10T23:47:12Z

About the breaking changes note: we generally don't consider constructors to be part of the public API so we may be able to land this in patch, but I'm also okay with just targeting major (v14) to be certain that we won't break people.

AndrewKushnir · 2022-03-22T00:39:55Z

Sorry for the late reply. I've looked at the change and I think this solution might be incompatible with the new bootstrap API that we plan to introduce to support Standalone Components (there will be a public RFC about those APIs soon), more specifically, the ApplicationRef will likely be created sooner during the bootstrap process, so it would break implemented ordering. We can look into this issue once again and try to come up with a more explicit way to fix it, once the bootstrap API for standalone components is implemented (I'll keep you updated).

arturovt · 2022-06-18T19:44:12Z

Hey @AndrewKushnir . I've rebased this PR on the main branch. This seems like tests are green with the new bootstrap API for standalone components. Are you able to run presubmit if those changes make sense?

packages/platform-browser/animations/test/animation_renderer_spec.ts

AndrewKushnir · 2022-06-22T05:16:16Z

Global Presubmit (internal-only link).

Currently, when importing `BrowserAnimationsModule`, Angular provides `AnimationRendererFactory` as the `RendererFactory2`. The `AnimationRendererFactory` relies on the `AnimationEngine`. The `AnimationEngine` may be created earlier than the `ApplicationRef` (e.g. if it's requested within the `APP_INITIALIZER` before the `ApplicationRef` is created). This means that Angular will add the `AnimationEngine` to `R3Injector.onDestroy` before the `ApplicationRef`. The `R3Injector` will call `ngOnDestroy()` on the `AnimationEngine` before the `ApplicationRef`, which means the `flush()` will be called earlier before views are destroyed. PR Close angular#45108

dylhunn · 2022-06-23T20:53:01Z

This PR was merged into the repository by commit 4eb1ca1.

…45143) Currently, when importing `BrowserAnimationsModule`, Angular provides `AnimationRendererFactory` as the `RendererFactory2`. The `AnimationRendererFactory` relies on the `AnimationEngine`. The `AnimationEngine` may be created earlier than the `ApplicationRef` (e.g. if it's requested within the `APP_INITIALIZER` before the `ApplicationRef` is created). This means that Angular will add the `AnimationEngine` to `R3Injector.onDestroy` before the `ApplicationRef`. The `R3Injector` will call `ngOnDestroy()` on the `AnimationEngine` before the `ApplicationRef`, which means the `flush()` will be called earlier before views are destroyed. PR Close #45108 PR Close #45143

This PR contains the following updates: | Package | Type | Update | Change | |---|---|---|---| | [@angular/animations](https://github.com/angular/angular) | dependencies | patch | [`14.0.3` -> `14.0.4`](https://renovatebot.com/diffs/npm/@angular%2fanimations/14.0.3/14.0.4) | | [@angular/common](https://github.com/angular/angular) | dependencies | patch | [`14.0.3` -> `14.0.4`](https://renovatebot.com/diffs/npm/@angular%2fcommon/14.0.3/14.0.4) | | [@angular/compiler](https://github.com/angular/angular) | dependencies | patch | [`14.0.3` -> `14.0.4`](https://renovatebot.com/diffs/npm/@angular%2fcompiler/14.0.3/14.0.4) | | [@angular/compiler-cli](https://github.com/angular/angular) | devDependencies | patch | [`14.0.3` -> `14.0.4`](https://renovatebot.com/diffs/npm/@angular%2fcompiler-cli/14.0.3/14.0.4) | | [@angular/core](https://github.com/angular/angular) | dependencies | patch | [`14.0.3` -> `14.0.4`](https://renovatebot.com/diffs/npm/@angular%2fcore/14.0.3/14.0.4) | | [@angular/forms](https://github.com/angular/angular) | dependencies | patch | [`14.0.3` -> `14.0.4`](https://renovatebot.com/diffs/npm/@angular%2fforms/14.0.3/14.0.4) | | [@angular/platform-browser](https://github.com/angular/angular) | dependencies | patch | [`14.0.3` -> `14.0.4`](https://renovatebot.com/diffs/npm/@angular%2fplatform-browser/14.0.3/14.0.4) | | [@angular/platform-browser-dynamic](https://github.com/angular/angular) | dependencies | patch | [`14.0.3` -> `14.0.4`](https://renovatebot.com/diffs/npm/@angular%2fplatform-browser-dynamic/14.0.3/14.0.4) | --- ### Release Notes <details> <summary>angular/angular</summary> ### [`v14.0.4`](https://github.com/angular/angular/blob/HEAD/CHANGELOG.md#1404-2022-06-29) [Compare Source](angular/angular@14.0.3...14.0.4) ##### animations | Commit | Type | Description | | -- | -- | -- | | [51be9bbe29](angular/angular@51be9bb) | fix | cleanup DOM elements when the root view is removed ([#45143](angular/angular#45143)) | | [999aca86c8](angular/angular@999aca8) | fix | enable shadowElements to leave when their parent does ([#46459](angular/angular#46459)) | ##### common | Commit | Type | Description | | -- | -- | -- | | [42aed6b13e](angular/angular@42aed6b) | fix | handle CSS custom properties in NgStyle ([#46451](angular/angular#46451)) | ##### core | Commit | Type | Description | | -- | -- | -- | | [1e7f22f00a](angular/angular@1e7f22f) | fix | trigger `ApplicationRef.destroy` when Platform is destroyed ([#46497](angular/angular#46497)) | | [8bde2dbc71](angular/angular@8bde2db) | fix | Update ngfor error code to be negative ([#46555](angular/angular#46555)) | | [57e8fc00eb](angular/angular@57e8fc0) | fix | Updates error to use RuntimeError code ([#46526](angular/angular#46526)) | ##### forms | Commit | Type | Description | | -- | -- | -- | | [74a26d870e](angular/angular@74a26d8) | fix | Convert existing reactive errors to use RuntimeErrorCode. ([#46560](angular/angular#46560)) | | [747872212d](angular/angular@7478722) | fix | Update a Forms validator error to use RuntimeError ([#46537](angular/angular#46537)) | ##### router | Commit | Type | Description | | -- | -- | -- | | [d6fac9e914](angular/angular@d6fac9e) | fix | Ensure that new `RouterOutlet` instances work after old ones are destroyed ([#46554](angular/angular#46554)) | #### Special Thanks Alan Agius, Alex Rickabaugh, Andrew Kushnir, Andrew Scott, Bezael, Chad Ramos, Chellappan, Cédric Exbrayat, Dylan Hunn, George Kalpakas, Jeremy Meiss, Jessica Janiuk, Joey Perrott, KMathy, Kristiyan Kostadinov, Paul Gschwendtner, Pawel Kozlowski, Ramesh Thiruchelvam, Vaibhav Kumar, arturovt, dario-piotrowicz and renovate\[bot]  </details> --- ### Configuration 📅 **Schedule**: Branch creation - At any time (no schedule defined), Automerge - At any time (no schedule defined). 🚦 **Automerge**: Disabled by config. Please merge this manually once you are satisfied. ♻ **Rebasing**: Whenever PR becomes conflicted, or you tick the rebase/retry checkbox. 🔕 **Ignore**: Close this PR and you won't be reminded about these updates again. --- - [ ] If you want to rebase/retry this PR, click this checkbox. --- This PR has been generated by [Renovate Bot](https://github.com/renovatebot/renovate). Co-authored-by: cabr2-bot <cabr2.help@gmail.com> Reviewed-on: https://codeberg.org/Calciumdibromid/CaBr2/pulls/1437 Reviewed-by: Epsilon_02 <epsilon_02@noreply.codeberg.org> Co-authored-by: Calciumdibromid Bot <cabr2_bot@noreply.codeberg.org> Co-committed-by: Calciumdibromid Bot <cabr2_bot@noreply.codeberg.org>

angular-automatic-lock-bot · 2022-07-24T16:06:30Z

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

pullapprove bot requested a review from alxhub February 19, 2022 18:36

angular-robot bot added the flag: breaking change label Feb 19, 2022

arturovt force-pushed the fix/45108 branch 3 times, most recently from 78c5737 to 7f50100 Compare February 23, 2022 20:33

JoostK linked an issue Feb 23, 2022 that may be closed by this pull request

DOM elements are not removed when the root view is removed if the RendererFactory2 is injected into the root provider #45108

Closed

JoostK added the action: cleanup The PR is in need of cleanup, either due to needing a rebase or in response to comments from reviews label Feb 23, 2022

jessicajaniuk added area: animations target: patch This PR is targeted for the next patch release labels Feb 24, 2022

ngbot bot modified the milestone: Backlog Feb 24, 2022

arturovt force-pushed the fix/45108 branch from 7f50100 to fd3010b Compare March 10, 2022 21:41

JoostK requested a review from jessicajaniuk March 10, 2022 23:31

JoostK added action: review The PR is still awaiting reviews from at least one requested reviewer and removed action: cleanup The PR is in need of cleanup, either due to needing a rebase or in response to comments from reviews labels Mar 10, 2022

JoostK reviewed Mar 10, 2022

View reviewed changes

packages/platform-browser/animations/test/animation_renderer_spec.ts Outdated Show resolved Hide resolved

packages/platform-browser/animations/test/animation_renderer_spec.ts Show resolved Hide resolved

jessicajaniuk previously approved these changes Mar 10, 2022

View reviewed changes

packages/platform-browser/animations/src/providers.ts Show resolved Hide resolved

JoostK removed the flag: breaking change label Mar 10, 2022

arturovt force-pushed the fix/45108 branch from fd3010b to a1638f0 Compare March 11, 2022 00:00

JoostK approved these changes Mar 11, 2022

View reviewed changes

JoostK added action: merge The PR is ready for merge by the caretaker 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 Mar 11, 2022

AndrewKushnir added the state: blocked label Mar 22, 2022

AndrewKushnir removed action: merge The PR is ready for merge by the caretaker action: presubmit The PR is in need of a google3 presubmit labels Mar 22, 2022

AndrewKushnir self-assigned this Mar 22, 2022

dylhunn dismissed jessicajaniuk’s stale review via a1638f0 May 2, 2022 18:42

arturovt force-pushed the fix/45108 branch from a1638f0 to fa9d581 Compare June 18, 2022 19:11

AndrewKushnir added action: global presubmit The PR is in need of a google3 global presubmit and removed state: blocked labels Jun 21, 2022

AndrewKushnir reviewed Jun 22, 2022

View reviewed changes

packages/platform-browser/animations/test/animation_renderer_spec.ts Show resolved Hide resolved

arturovt force-pushed the fix/45108 branch from fa9d581 to b014d91 Compare June 22, 2022 11:01

AndrewKushnir approved these changes Jun 22, 2022

View reviewed changes

AndrewKushnir added action: merge The PR is ready for merge by the caretaker and removed action: global presubmit The PR is in need of a google3 global presubmit labels Jun 22, 2022

jessicajaniuk approved these changes Jun 22, 2022

View reviewed changes

jessicajaniuk removed the request for review from alxhub June 22, 2022 21:18

dylhunn closed this in 4eb1ca1 Jun 23, 2022

arturovt deleted the fix/45108 branch June 23, 2022 21:03

This was referenced Jul 1, 2022

build(deps-dev): bump @angular/language-service from 12.2.4 to 14.0.4 numbersprotocol/capture-lite#1761

Closed

build(deps-dev): bump @angular/compiler from 12.2.4 to 14.0.4 numbersprotocol/capture-lite#1755

Closed

angular-automatic-lock-bot bot locked and limited conversation to collaborators Jul 24, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(animations): cleanup DOM elements when the root view is removed #45143

fix(animations): cleanup DOM elements when the root view is removed #45143

arturovt commented Feb 19, 2022

JoostK commented Feb 23, 2022

arturovt commented Feb 23, 2022 •

edited

arturovt commented Feb 24, 2022

JoostK commented Feb 24, 2022

arturovt commented Mar 10, 2022

JoostK left a comment

jessicajaniuk left a comment

JoostK commented Mar 10, 2022

AndrewKushnir commented Mar 22, 2022 •

edited

arturovt commented Jun 18, 2022

AndrewKushnir commented Jun 22, 2022

dylhunn commented Jun 23, 2022

angular-automatic-lock-bot bot commented Jul 24, 2022

fix(animations): cleanup DOM elements when the root view is removed #45143

fix(animations): cleanup DOM elements when the root view is removed #45143

Conversation

arturovt commented Feb 19, 2022

PR Checklist

PR Type

What is the current behavior?

JoostK commented Feb 23, 2022

arturovt commented Feb 23, 2022 • edited

arturovt commented Feb 24, 2022

JoostK commented Feb 24, 2022

arturovt commented Mar 10, 2022

JoostK left a comment

Choose a reason for hiding this comment

jessicajaniuk left a comment

Choose a reason for hiding this comment

JoostK commented Mar 10, 2022

AndrewKushnir commented Mar 22, 2022 • edited

arturovt commented Jun 18, 2022

AndrewKushnir commented Jun 22, 2022

dylhunn commented Jun 23, 2022

angular-automatic-lock-bot bot commented Jul 24, 2022

arturovt commented Feb 23, 2022 •

edited

AndrewKushnir commented Mar 22, 2022 •

edited