fix(core): ensure init lifecycle events are called #20258

chuckjaz · 2017-11-08T01:22:48Z

Throwing an exception in a lifecycle event will delay but not
prevent an Init method, such as ngOnInit, ngAfterContentInit,
or ngAfterViewInit, from being called. Also, calling detectChanges()
in a way that causes duplicate change detection (such as a
child component causing a parent to call detectChanges() on its
own ChangeDetectorRef, will no longer prevent change ngOnInit,
ngAfterContentInit and ngAfterViewInit from being called.

With this change lifecycle methods are still not guarenteed to be
called but the Init methods will be called if at least one change
detection pass on its view is completed.

Fixes: #17035

PR Type

What kind of change does this PR introduce?

[x] Bugfix

What is the current behavior?

Issue Number: #17035

Initialization lifecycle methods might not be called on an component even after a successful check of its view has been completed.

What is the new behavior?

The successful completion of a check of a view will ensure that all components and directives will have the initialization methods, such as ngOnInit, ngAfterContentInit and ngAfterViewInit, are called once an only once.

Does this PR introduce a breaking change?

[ ] Yes
[x] No

mary-poppins · 2017-11-08T01:35:58Z

You can preview 75fd092 at https://pr20258-75fd092.ngbuilds.io/.

vicb · 2017-11-08T01:40:42Z

packages/core/src/view/types.ts

+// lifecycle methods.
+export function checkCycle(view: ViewData, cycle: ViewState): boolean {
+  const state = view.state;
+  if ((state & ViewState.InitStateMask) === ((cycle - 1) & ViewState.InitStateMask)) {


Either this code is way too smart for me to understand w/o thinking for 2h or it is wrong...

which in either case is probably a code smell

Either way it should have a comment. I will add one.

chuckjaz · 2017-11-08T02:45:28Z

Changes need to land in G3 before this can land.

mary-poppins · 2017-11-08T03:11:01Z

You can preview ba73309 at https://pr20258-ba73309.ngbuilds.io/.

mary-poppins · 2017-11-08T21:15:40Z

You can preview 47f5027 at https://pr20258-47f5027.ngbuilds.io/.

vicb · 2017-11-09T00:31:26Z

packages/core/src/view/provider.ts

  for (let i = 0; i < nodes.length; i++) {
    const nodeDef = nodes[i];
    let parent = nodeDef.parent;
    if (!parent && nodeDef.flags & lifecycles) {
      // matching root node (e.g. a pipe)
-      callProviderLifecycles(view, i, nodeDef.flags & lifecycles);
+      callProviderLifecycles(view, i, nodeDef.flags & lifecycles, initIndex++);


nit(style): what about ++ing initIndex after the call ?

This is a a performance and size critical part of the code. Moving the ++ after the call increases the size of this expression in a way that is impossible for closure or rollup to optimize so I would prefer to keep the current code.

If this is critical there should be a comment IMO.

vicb · 2017-11-09T00:31:53Z

packages/core/src/view/provider.ts

  for (let i = elDef.nodeIndex + 1; i <= elDef.nodeIndex + elDef.childCount; i++) {
    const nodeDef = view.def.nodes[i];
    if (nodeDef.flags & lifecycles) {
-      callProviderLifecycles(view, i, nodeDef.flags & lifecycles);
+      callProviderLifecycles(view, i, nodeDef.flags & lifecycles, initIndex++);


same comment here

vicb · 2017-11-09T00:34:33Z

packages/core/src/view/types.ts

+  // thrown while checkAndUpdateView is running, checkAndUpdateView starts over
+  // from the beginning. This ensures the state is monotonically increasing,
+  // terminating in the AfterInit state, which ensures the Init methods are called
+  // atleast once and only once.


minor typo at+ +least

use backquotes for code refs in the comments above ?

vicb · 2017-11-09T00:43:19Z

packages/core/test/linker/change_detection_integration_spec.ts

+      }
+
+      function forEachMethod(methods: LifetimeMethods, cb: (method: LifetimeMethods) => void) {
+        for (let method = LifetimeMethods.ngOnInit; method <= LifetimeMethods.All; method <<= 1) {


method <= LifetimeMethods.All is too smart for me.
Probably the semantic is not correct ? should it be method <= LifetimeMethods. ngDoCheck

I would go for simple and more maintainable version for(const method of [explicit list])

mary-poppins · 2017-11-09T01:16:52Z

You can preview 2e21554 at https://pr20258-2e21554.ngbuilds.io/.

vicb · 2017-11-09T18:18:41Z

packages/core/src/view/provider.ts

@@ -198,7 +198,8 @@ export function checkAndUpdateDirectiveInline(
  if (changes) {
    directive.ngOnChanges(changes);
  }
-  if ((view.state & ViewState.FirstCheck) && (def.flags & NodeFlags.OnInit)) {
+  if ((def.flags & NodeFlags.OnInit) &&
+      shouldCallInitHook(view, ViewState.CallingInit, def.nodeIndex)) {


call it shouldCallInitLifeCycleHook ?

ViewState.(Calling)OnInit should rather be a hook names ?

Throwing an exception in a lifecycle event will delay but not prevent an Init method, such as `ngOnInit`, `ngAfterContentInit`, or `ngAfterViewInit`, from being called. Also, calling `detectChanges()` in a way that causes duplicate change detection (such as a child component causing a parent to call `detectChanges()` on its own `ChangeDetectorRef`, will no longer prevent change `ngOnInit`, `ngAfterContentInit` and `ngAfterViewInit` from being called. With this change lifecycle methods are still not guarenteed to be called but the Init methods will be called if at least one change detection pass on its view is completed. Fixes: angular#17035

vicb · 2017-11-09T20:53:44Z

packages/core/src/view/provider.ts

+// the view will begin getting ngAfterViewInit() called until a check and
+// pass is complete.
+//
+// This algorthim also handles recursion. Consider if E4's ngAfterViewInit()


typo algorithm

vicb · 2017-11-09T20:54:37Z

packages/core/src/view/provider.ts

+// initIndex is set to 6, the recusive checkAndUpdateView() starts walk again.
+// D3, E2, E6, E7, E5 and E4 are skipped, ngAfterViewInit() is called on E1.
+// When the recursion returns the initIndex will be 7 so E1 is skipped as it
+// has already been called in the recursively called checkAnUpdateView().


👍
Love this comment block, very helpful.

mary-poppins · 2017-11-09T21:23:44Z

You can preview 1018b57 at https://pr20258-1018b57.ngbuilds.io/.

Throwing an exception in a lifecycle event will delay but not prevent an Init method, such as `ngOnInit`, `ngAfterContentInit`, or `ngAfterViewInit`, from being called. Also, calling `detectChanges()` in a way that causes duplicate change detection (such as a child component causing a parent to call `detectChanges()` on its own `ChangeDetectorRef`, will no longer prevent change `ngOnInit`, `ngAfterContentInit` and `ngAfterViewInit` from being called. With this change lifecycle methods are still not guarenteed to be called but the Init methods will be called if at least one change detection pass on its view is completed. Fixes: angular#17035 PR Close angular#20258

angular-automatic-lock-bot · 2019-09-12T20:52:44Z

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 Nov 8, 2017

vicb reviewed Nov 8, 2017

View reviewed changes

vicb added action: review The PR is still awaiting reviews from at least one requested reviewer area: core Issues related to the framework runtime labels Nov 8, 2017

chuckjaz added the state: blocked label Nov 8, 2017

chuckjaz force-pushed the issue17035 branch from 75fd092 to ba73309 Compare November 8, 2017 02:59

chuckjaz force-pushed the issue17035 branch from ba73309 to 47f5027 Compare November 8, 2017 21:06

vicb reviewed Nov 9, 2017

View reviewed changes

chuckjaz force-pushed the issue17035 branch from 47f5027 to 2e21554 Compare November 9, 2017 01:08

vicb reviewed Nov 9, 2017

View reviewed changes

vicb approved these changes Nov 9, 2017

View reviewed changes

vicb removed action: review The PR is still awaiting reviews from at least one requested reviewer state: blocked labels Nov 9, 2017

chuckjaz force-pushed the issue17035 branch from 2e21554 to 1018b57 Compare November 9, 2017 20:48

vicb reviewed Nov 9, 2017

View reviewed changes

chuckjaz added type: bug/fix target: major This PR is targeted for the next major release action: merge The PR is ready for merge by the caretaker labels Nov 10, 2017

jasonaden closed this in 24cf8b3 Nov 10, 2017

mark-langer mentioned this pull request Nov 27, 2017

Lifecycle hooks are not called on router-loaded child components when parent's change detector is detached #17087

Closed

chuckjaz mentioned this pull request Dec 5, 2017

Component lifecycle Init functions not always called before associated check function #18631

Closed

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(core): ensure init lifecycle events are called #20258

fix(core): ensure init lifecycle events are called #20258

chuckjaz commented Nov 8, 2017 •

edited by vicb

Loading

mary-poppins commented Nov 8, 2017

vicb Nov 8, 2017

vicb Nov 8, 2017

chuckjaz Nov 8, 2017

chuckjaz commented Nov 8, 2017

mary-poppins commented Nov 8, 2017

mary-poppins commented Nov 8, 2017

vicb Nov 9, 2017

chuckjaz Nov 9, 2017

vicb Nov 9, 2017 •

edited

Loading

vicb Nov 9, 2017

vicb Nov 9, 2017

vicb Nov 9, 2017

mary-poppins commented Nov 9, 2017

vicb Nov 9, 2017 •

edited

Loading

vicb Nov 9, 2017

vicb Nov 9, 2017

mary-poppins commented Nov 9, 2017

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

fix(core): ensure init lifecycle events are called #20258

fix(core): ensure init lifecycle events are called #20258

Conversation

chuckjaz commented Nov 8, 2017 • edited by vicb Loading

PR Type

What is the current behavior?

What is the new behavior?

Does this PR introduce a breaking change?

mary-poppins commented Nov 8, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chuckjaz commented Nov 8, 2017

mary-poppins commented Nov 8, 2017

mary-poppins commented Nov 8, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vicb Nov 9, 2017 • edited Loading

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 Nov 9, 2017

vicb Nov 9, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mary-poppins commented Nov 9, 2017

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

chuckjaz commented Nov 8, 2017 •

edited by vicb

Loading

vicb Nov 9, 2017 •

edited

Loading

vicb Nov 9, 2017 •

edited

Loading