feat(ivy): support lifecycle hooks of ViewContainerRef #23396

marclaval · 2018-04-16T14:19:56Z

The goal of this PR is to support lifecycle hooks of ViewContainerRef.
The 2 main changes:

the refreshDirectives method has been renamed refreshView and now includes the call to refreshDynamicChildren so that it can be correctly done between init hooks calls and content hooks calls, as in Angular today.
renderEmbeddedTemplate is called twice: once in creation mode only and once in update mode only. This has to be taken into account as a few things must be done during/after the first update: calling refreshView, call view hooks, setting creationMode to false.

The extra test in lifecycle_spec.ts is to to ensure that the new engine correctly handles lifecycle hooks when there is a component in the content.

mary-poppins · 2018-04-16T14:30:20Z

You can preview f0d1403 at https://pr23396-f0d1403.ngbuilds.io/.

mary-poppins · 2018-04-16T16:55:44Z

You can preview eea7834 at https://pr23396-eea7834.ngbuilds.io/.

JoostK · 2018-04-16T18:42:18Z

packages/core/test/render3/view_container_ref_spec.ts

+    it('should call all hooks in correct order', () => {
+      @Component({selector: 'hooks', template: `{{name}}`})
+      class ComponentWithHooks {
+        name: string;


This is missing an @Input decorator. Although it won't have any effect, it will make the ngComponentDef in line with the decorators.

mary-poppins · 2018-04-18T15:07:13Z

You can preview 827bd2e at https://pr23396-827bd2e.ngbuilds.io/.

mary-poppins · 2018-04-18T15:51:08Z

You can preview 8015e38 at https://pr23396-8015e38.ngbuilds.io/.

vicb · 2018-04-18T17:21:29Z

packages/core/src/render3/instructions.ts

-  if (!checkNoChangesMode) {
-    executeHooks(
-        directives !, currentView.tView.viewHooks, currentView.tView.viewCheckHooks, creationMode);
+export function leaveView(newView: LView, creationOnly?: boolean): void {


Add API docs for the new parameter

vicb · 2018-04-18T17:22:43Z

packages/core/src/render3/instructions.ts

+export function leaveView(newView: LView, creationOnly?: boolean): void {
+  if (!creationOnly) {
+    if (!checkNoChangesMode) {
+      executeHooks(


Add an assert in executeHooks ? (!checkNoChangesMode)

checkNoChangesMode is not accessible in executeHooks, I don't think we want to pass it or add a dependency

vicb · 2018-04-18T17:23:07Z

packages/core/src/render3/instructions.ts

+          directives !, currentView.tView.viewHooks, currentView.tView.viewCheckHooks,
+          creationMode);
+    }
+    // Views should be clean and in update mode after being checked, so these bits are cleared


should be -> is ?

vicb · 2018-04-18T17:23:53Z

packages/core/src/render3/instructions.ts

-function refreshDirectives() {
-  executeInitAndContentHooks();
-
+/**  Refreshes the view: dynamic children and directives, triggering any init/content hooks.  */


expand the comment a little bit ?

vicb · 2018-04-18T17:26:34Z

packages/core/src/render3/instructions.ts

  } finally {
-    leaveView(oldView !);
+    leaveView(oldView !, (rf & RenderFlags.Create) === RenderFlags.Create);


suggestion:

// comment ? const creationMode = (rf & RenderFlags.Create) === RenderFlags.Create; leaveView(oldView !, creationMode);

mary-poppins · 2018-04-19T10:06:58Z

You can preview 788de25 at https://pr23396-788de25.ngbuilds.io/.

mhevery · 2018-04-19T17:39:49Z

@kara could you review this.

kara

Some nits and a question

kara · 2018-04-19T20:29:55Z

packages/core/src/render3/instructions.ts

  currentView.lifecycleStage = LifecycleStage.Init;
  currentView.bindingIndex = -1;
  enterView(newView, null);
 }

-/**  Refreshes directives in this view and triggers any init/content hooks.  */
-function refreshDirectives() {
-  executeInitAndContentHooks();


If we're removing this usage, we should probably remove the executeInitAndContentHooks function and inline it. I think it's only used in one other place.

It is still used in 2 different places: renderComponent and renderComponentOrTemplate. If this can be refactored, let's do it in a future PR.

Aha, forgot to check component.ts. In that case, sg.

kara · 2018-04-19T20:30:50Z

packages/core/src/render3/instructions.ts

+ *
+ * @param newView New state to become active
+ * @param creationOnly An optional boolean to indicate that the view was processed in creation mode
+ * only, i.e. the first update will be done later


Nit: Maybe clarify to indicate that the separate modes occur for dynamically created views ?

kara · 2018-04-19T20:31:54Z

packages/core/src/render3/instructions.ts

+          creationMode);
+    }
+    // Views are clean and in update mode after being checked, so these bits are cleared
+    currentView.flags &= ~(LViewFlags.CreationMode | LViewFlags.Dirty);


It seems like we should be clearing the creation flag whenever we leave a view, even if we are creationOnly. Otherwise, the creation flag would be on for the update mode check that comes next. Can you clarify why it should be in creation mode for both runs?

If the first update is not done with the CreationMode flag still on, then the onInit, afterContentInit and afterViewInit hooks are never called.

Ah, thanks for explaining, I get what you're doing now.

My only concern is that the RenderFlags and ViewFlags are sometimes out of sync. The second time through, we are in update mode according to RenderFlags, but we are still in creation mode according to ViewFlags. What do you think about keeping the view flags in sync (by clearing the creation mode in ViewFlags), but using the existing lifecycleStage property to track hooks?

If that doesn't work, okay to keep it as is, as long as we add a comment/TODO explaining the discrepancy.

kara · 2018-04-19T20:46:57Z

packages/core/test/render3/lifecycle_spec.ts

@@ -2436,6 +2436,90 @@ describe('lifecycles', () => {

    });

+    // Angular 5 reference: https://stackblitz.com/edit/lifecycle-hooks-ng


Great idea to include a stackblitz example in a comment 👍

kara · 2018-04-19T20:47:41Z

packages/core/test/render3/lifecycle_spec.ts

+      function Template(rf: RenderFlags, ctx: any) {
+        if (rf & RenderFlags.Create) {
+          elementStart(0, 'parent');
+          elementStart(1, 'content');


Nit: any reason why we're not indenting the inner ones to make it a little easier to read?

mary-poppins · 2018-04-24T00:21:18Z

You can preview 331902c at https://pr23396-331902c.ngbuilds.io/.

vicb · 2018-04-25T18:34:06Z

packages/core/src/render3/instructions.ts

+ * @param newView New state to become active
+ * @param creationOnly An optional boolean to indicate that the view was processed in creation mode
+ * only, i.e. the first update will be done later. Only possible for dynamically created views.
+ * @returns the previous state


Actually returns void

vicb

@marclaval please fix the return type (should be void).

mary-poppins · 2018-04-25T18:57:09Z

You can preview 8dc9705 at https://pr23396-8dc9705.ngbuilds.io/.

IgorMinar

lgtm for the json file changes.

@kara please send the PR to exclude these

IgorMinar · 2018-04-25T23:02:42Z

    http://test/OCL:194312401:BASE:194313007:1524697346415:f5b35693

angular-automatic-lock-bot · 2019-09-13T14:21:11Z

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

googlebot added the cla: yes label Apr 16, 2018

marclaval added action: review The PR is still awaiting reviews from at least one requested reviewer target: major This PR is targeted for the next major release comp: ivy labels Apr 16, 2018

marclaval force-pushed the vcref_lifecyclehooks branch from f0d1403 to eea7834 Compare April 16, 2018 16:45

JoostK reviewed Apr 16, 2018

View reviewed changes

marclaval force-pushed the vcref_lifecyclehooks branch from eea7834 to 827bd2e Compare April 18, 2018 14:55

marclaval force-pushed the vcref_lifecyclehooks branch from 827bd2e to 8015e38 Compare April 18, 2018 15:36

vicb reviewed Apr 18, 2018

View reviewed changes

mhevery requested a review from kara April 19, 2018 17:39

kara reviewed Apr 19, 2018

View reviewed changes

marclaval force-pushed the vcref_lifecyclehooks branch from 788de25 to 331902c Compare April 23, 2018 23:54

marclaval added 3 commits April 24, 2018 15:37

feat(ivy): support lifecycle hooks of ViewContainerRef

f39b572

fixup! feat(ivy): support lifecycle hooks of ViewContainerRef

c45d28c

fixup! feat(ivy): support lifecycle hooks of ViewContainerRef

c460570

vicb reviewed Apr 25, 2018

View reviewed changes

vicb approved these changes Apr 25, 2018

View reviewed changes

fixup! feat(ivy): support lifecycle hooks of ViewContainerRef

8dc9705

marclaval force-pushed the vcref_lifecyclehooks branch from 331902c to 8dc9705 Compare April 25, 2018 18:45

marclaval 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 Apr 25, 2018

IgorMinar approved these changes Apr 25, 2018

View reviewed changes

IgorMinar closed this in 1a44a0b Apr 26, 2018

angular-automatic-lock-bot bot locked and limited conversation to collaborators Sep 13, 2019

		@@ -2436,6 +2436,90 @@ describe('lifecycles', () => {

		});

		// Angular 5 reference: https://stackblitz.com/edit/lifecycle-hooks-ng

feat(ivy): support lifecycle hooks of ViewContainerRef #23396

feat(ivy): support lifecycle hooks of ViewContainerRef #23396

Conversation

marclaval commented Apr 16, 2018

mary-poppins commented Apr 16, 2018

mary-poppins commented Apr 16, 2018

Choose a reason for hiding this comment

mary-poppins commented Apr 18, 2018

mary-poppins commented Apr 18, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mary-poppins commented Apr 19, 2018

mhevery commented Apr 19, 2018

kara left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kara Apr 24, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mary-poppins commented Apr 24, 2018

Choose a reason for hiding this comment

vicb left a comment

Choose a reason for hiding this comment

mary-poppins commented Apr 25, 2018

IgorMinar left a comment

Choose a reason for hiding this comment

IgorMinar commented Apr 25, 2018

angular-automatic-lock-bot bot commented Sep 13, 2019

kara Apr 24, 2018 •

edited