refactor(core): separate LView-level and TView-level destroy hooks #49158

pkozlowski-opensource · 2023-02-22T11:15:00Z

No description provided.

packages/core/src/render3/interfaces/view.ts

packages/core/src/render3/node_manipulation.ts

packages/core/src/linker/destroy_ref.ts

Harpush · 2023-02-24T15:19:40Z

@pkozlowski-opensource Does this allow for destroy logic in inject based functions without hacks (inject(ChangeDetectorRef) as ViewRef)? If it is please make it public API.

gmfun · 2023-02-25T08:04:24Z

@pkozlowski-opensource Does this allow for destroy logic in inject based functions without hacks (inject(ChangeDetectorRef) as ViewRef)? If it is please make it public API.

inject(DestroyRef).onDestroy is available now as per the tests

packages/core/src/linker/destroy_ref.ts

atscott

reviewed-for: public-api, circular-deps

packages/core/src/linker/destroy_ref.ts

packages/core/src/render3/instructions/shared.ts

packages/core/src/render3/interfaces/view.ts

jelbourn · 2023-02-28T16:45:07Z

packages/core/src/render3/interfaces/view.ts

+   * are user defined, LView-specific destroy callbacks that don't have any corresponding TView
+   * entries.
+   */
+  [ON_DESTROY_HOOKS]: Array<() => void>|null;


If would be convenient to mention here how the user defines these callsbacks. This would be useful for newcomers to this codebase.

packages/core/src/render3/node_manipulation.ts

packages/core/test/acceptance/destroy_ref_spec.ts

packages/core/src/di/r3_injector.ts

dylhunn

Looks very nice.
reviewed-for: public-api, fw-core

DestroyRef represents a concept of lifecycle scope where destroy callbacks can be registered. Such callbacks are automatically executed when a given scope ends it lifecycle. In practice the most common lifecycle scopes would be represented by: - a component or en embedded view; - instance of `EnvironnementInjector`.

alxhub

Reviewed-for: size-tracking

AndrewKushnir

FYI, just a minor (non-blocking) comment, which can be addressed in a followup PR (if needed). I will proceed with merging this PR.

AndrewKushnir · 2023-02-28T19:41:24Z

packages/core/src/linker/destroy_ref.ts

+ * directive is destroyed. Otherwise the callbacks run when a corresponding injector is destroyed.
+ */


Suggested change

* directive is destroyed. Otherwise the callbacks run when a corresponding injector is destroyed.

*/

* directive is destroyed. Otherwise the callbacks run when a corresponding injector is destroyed.

*

* @publicApi

*/

Do we want "Developer Preview" label here as well?

Actually, I'm not sure :-) I do think that we might want to have this API regardless of other dev-preview work where it is going to be used primarily.

But yep, this is good discussion to be had! I would appreciate if we could get it in (was trying to make it green for a while...) and decide on the API flag in a separate PR.

Thnx for checking @AndrewKushnir !

AndrewKushnir · 2023-02-28T19:52:12Z

This PR was merged into the repository by commit 0f5c800.

ajafff · 2023-03-17T08:13:43Z

goldens/public-api/core/index.md

@@ -436,6 +436,11 @@ export const defineInjectable: typeof ɵɵdefineInjectable;
 // @public
 export function destroyPlatform(): void;

+// @public
+export abstract class DestroyRef {
+    abstract onDestroy(callback: () => void): void;


how would I unregister the callback to avoid leaking memory in long-lived DestroyRefs?

We are going to add API for this in a follow-up PR

angular-automatic-lock-bot · 2023-04-17T00:08:19Z

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

pkozlowski-opensource added the area: core Issues related to the framework runtime label Feb 22, 2023

ngbot bot added this to the Backlog milestone Feb 22, 2023

pkozlowski-opensource added the action: review The PR is still awaiting reviews from at least one requested reviewer label Feb 22, 2023

pkozlowski-opensource marked this pull request as draft February 22, 2023 11:21

pkozlowski-opensource force-pushed the lview_on_destroy branch from 29350ed to a8df323 Compare February 22, 2023 11:38

angular-robot bot added the detected: feature PR contains a feature commit label Feb 22, 2023

pkozlowski-opensource force-pushed the lview_on_destroy branch 4 times, most recently from ce20ab6 to 6280861 Compare February 22, 2023 15:36

pkozlowski-opensource marked this pull request as ready for review February 22, 2023 15:49

pullapprove bot requested review from alxhub, atscott and dylhunn February 22, 2023 15:49

atscott requested changes Feb 22, 2023

View reviewed changes

packages/core/src/render3/interfaces/view.ts Outdated Show resolved Hide resolved

packages/core/src/render3/node_manipulation.ts Show resolved Hide resolved

pkozlowski-opensource force-pushed the lview_on_destroy branch 2 times, most recently from f77f448 to 1873f29 Compare February 22, 2023 17:12

atscott reviewed Feb 22, 2023

View reviewed changes

packages/core/src/linker/destroy_ref.ts Show resolved Hide resolved

pkozlowski-opensource force-pushed the lview_on_destroy branch from 1873f29 to 929d83d Compare February 22, 2023 17:39

pkozlowski-opensource mentioned this pull request Feb 22, 2023

Prototype of the RxJS interop layer for signals #49154

Closed

eneajaho reviewed Feb 22, 2023

View reviewed changes

packages/core/src/linker/destroy_ref.ts Outdated Show resolved Hide resolved

jelbourn reviewed Feb 22, 2023

View reviewed changes

packages/core/src/linker/destroy_ref.ts Outdated Show resolved Hide resolved

packages/core/src/linker/destroy_ref.ts Show resolved Hide resolved

pkozlowski-opensource force-pushed the lview_on_destroy branch from 929d83d to 9bd5091 Compare February 23, 2023 10:20

pkozlowski-opensource mentioned this pull request Feb 23, 2023

feat(core): Mark components for check if they read a signal #49153

Closed

elgreco247 reviewed Feb 25, 2023

View reviewed changes

packages/core/src/linker/destroy_ref.ts Outdated Show resolved Hide resolved

atscott approved these changes Feb 27, 2023

View reviewed changes

pullapprove bot requested a review from atscott February 27, 2023 17:11

atscott approved these changes Feb 27, 2023

View reviewed changes

pullapprove bot requested a review from jessicajaniuk February 27, 2023 17:15

pkozlowski-opensource force-pushed the lview_on_destroy branch from 9bd5091 to bc96532 Compare February 28, 2023 12:14

jelbourn reviewed Feb 28, 2023

View reviewed changes

alxhub approved these changes Feb 28, 2023

View reviewed changes

packages/core/src/di/r3_injector.ts Outdated Show resolved Hide resolved

pullapprove bot requested a review from alxhub February 28, 2023 17:08

dylhunn approved these changes Feb 28, 2023

View reviewed changes

pkozlowski-opensource force-pushed the lview_on_destroy branch from bc96532 to 348c8a8 Compare February 28, 2023 18:42

pkozlowski-opensource force-pushed the lview_on_destroy branch from 348c8a8 to 7bd5a82 Compare February 28, 2023 19:10

pkozlowski-opensource added the target: major This PR is targeted for the next major release label Feb 28, 2023

pkozlowski-opensource removed the request for review from jessicajaniuk February 28, 2023 19:27

pullapprove bot requested a review from dylhunn February 28, 2023 19:27

alxhub approved these changes Feb 28, 2023

View reviewed changes

pkozlowski-opensource removed the request for review from dylhunn February 28, 2023 19:33

pkozlowski-opensource added action: merge The PR is ready for merge by the caretaker and removed action: review The PR is still awaiting reviews from at least one requested reviewer labels Feb 28, 2023

AndrewKushnir reviewed Feb 28, 2023

View reviewed changes

AndrewKushnir closed this in 0f5c800 Feb 28, 2023

ajafff reviewed Mar 17, 2023

View reviewed changes

angular-automatic-lock-bot bot locked and limited conversation to collaborators Apr 17, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor(core): separate LView-level and TView-level destroy hooks #49158

refactor(core): separate LView-level and TView-level destroy hooks #49158

pkozlowski-opensource commented Feb 22, 2023

Harpush commented Feb 24, 2023

gmfun commented Feb 25, 2023

atscott left a comment

jelbourn Feb 28, 2023

dylhunn left a comment

alxhub left a comment

AndrewKushnir left a comment

AndrewKushnir Feb 28, 2023

AndrewKushnir Feb 28, 2023

pkozlowski-opensource Feb 28, 2023

AndrewKushnir commented Feb 28, 2023

ajafff Mar 17, 2023

pkozlowski-opensource Mar 17, 2023

angular-automatic-lock-bot bot commented Apr 17, 2023

		* directive is destroyed. Otherwise the callbacks run when a corresponding injector is destroyed.
		*/

refactor(core): separate LView-level and TView-level destroy hooks #49158

refactor(core): separate LView-level and TView-level destroy hooks #49158

Conversation

pkozlowski-opensource commented Feb 22, 2023

Harpush commented Feb 24, 2023

gmfun commented Feb 25, 2023

atscott left a comment

Choose a reason for hiding this comment

jelbourn Feb 28, 2023

Choose a reason for hiding this comment

dylhunn left a comment

Choose a reason for hiding this comment

alxhub left a comment

Choose a reason for hiding this comment

AndrewKushnir left a comment

Choose a reason for hiding this comment

AndrewKushnir Feb 28, 2023

Choose a reason for hiding this comment

AndrewKushnir Feb 28, 2023

Choose a reason for hiding this comment

pkozlowski-opensource Feb 28, 2023

Choose a reason for hiding this comment

AndrewKushnir commented Feb 28, 2023

ajafff Mar 17, 2023

Choose a reason for hiding this comment

pkozlowski-opensource Mar 17, 2023

Choose a reason for hiding this comment

angular-automatic-lock-bot bot commented Apr 17, 2023