fix(upgrade): fix/improve support for lifecycle hooks #13020

gkalpak · 2016-11-22T00:50:15Z

Please check if the PR fulfills these requirements

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)
~~Docs have been added / updated (for bug fixes / features)~~

What kind of change does this PR introduce? (check one with "x")

[x] Bugfix
[x] Feature
[ ] Code style update (formatting, local variables)
[ ] Refactoring (no functional changes, no api changes)
[ ] Build related changes
[ ] CI related changes
[ ] Other... Please describe:

What is the current behavior? (You can also link to an open issue here)

Lifecycle hooks are called on the "binding destination" (either the controller instance or the scope depending on the value of bindToController).
The $onInit() lifecycle hook is called after the linking phase.
The $postLink() lifecycle hook is not supported.

What is the new behavior?

With the exception of $onChanges, all lifecycle hooks are called on the controller instance, regardless of the value of bindToController (as happens in ng1).
The $onInit() lifecycle hook is called before the linking phase (as happens in ng1).
The $postLink lifecycle hook is supported.

Does this PR introduce a breaking change? (check one with "x")

[ ] Yes
[ ] No
[x] Maybe

If this PR contains a breaking change, please describe the impact and migration path for existing applications:

Theoretically, this is a fix. But if an application was relying on the previous (inconsistent with ng1) behavior and timing (which they shouldn't), it might break.

gkalpak · 2016-11-22T00:57:50Z

BTW, this is a follow-up to #12875.

chuckjaz · 2016-11-22T19:16:08Z

Please update the commit comments to use "fix(upgrade):" as a prefix instead of "fix(UpgradeNg1Component):"

gkalpak · 2016-11-23T10:15:17Z

Rebased (to account for 9092680) and updated commit message scopes.

vicb · 2016-12-09T23:10:33Z

@gkalpak @petebacondarwin what is the status here ?

gkalpak · 2016-12-10T08:43:22Z

It needs to be reviewed 😃

mgol · 2016-12-14T10:40:54Z

modules/@angular/upgrade/src/upgrade_ng1_adapter.ts

@@ -313,6 +326,12 @@ class UpgradeNg1ComponentAdapter implements OnInit, OnChanges, DoCheck {
    return controller;
  }

+  private callLifecycleHook(method: LifecycleHook, context: IBindingDestination, arg?: any) {


Why is this indirection needed?

Just avoiding repetition.

Ah, because of the if. OK.

mgol · 2016-12-14T10:45:18Z

modules/@angular/upgrade/test/upgrade_spec.ts

+             const element = html(`<div><ng2></ng2></div>`);
+             adapter.bootstrap(element, ['ng1']).ready((ref) => {
+               expect($postLinkSpyA).toHaveBeenCalled();
+               expect($postLinkSpyB).toHaveBeenCalled();


In many test cases, like this one, it's not actually verified if the hooks are called in a proper moment during the lifecycle of the component, just that they're called. I feel more thorough tests are needed.

👍
There is a test for the AoT version that at least verifies the relative order of lifecycle hooks (it is not perfect, but it is better than not having it). Maybe we should port it over and also expand the current tests.
_{(But this is not directly related to this PR.)}

The order is nice but it still doesn't verify the hooks are called in proper moments (i.e. after all prerequisite operations for calling them have been finished).

But I agree it's not necessarily related to this PR.

mgol · 2016-12-14T11:33:24Z

modules/@angular/upgrade/src/upgrade_ng1_adapter.ts

@@ -313,6 +326,12 @@ class UpgradeNg1ComponentAdapter implements OnInit, OnChanges, DoCheck {
    return controller;
  }

+  private callLifecycleHook(method: LifecycleHook, context: IBindingDestination, arg?: any) {


Ah, because of the if. OK.

petebacondarwin

I think this is good enough to merge. I left a few minor comments that can be fixed up in subsequent PRs. In particular I think we should have a test that logs all the hook calls to some array so that we can easily see the order of the hooks.

petebacondarwin · 2016-12-19T09:32:54Z

modules/@angular/upgrade/test/upgrade_spec.ts

+                                      bindToController: false,
+                                      controllerAs: '$ctrl',
+                                      controller: function() { this.$onInit = $onInitSpyB; }
+                                    }))


My understanding here is that you want to test the two scenarios where $onInit is provided on the controller instance and on the controller's prototype. So why does one have bindToController set to true and the other one set to false?

See https://github.com/angular/angular/pull/13020/files#r93101114.

petebacondarwin · 2016-12-19T09:34:33Z

modules/@angular/upgrade/test/upgrade_spec.ts

+                                      controller: function($scope: angular.IScope) {
+                                        $scope['$onInit'] = $onInitSpy;
+                                      }
+                                    }))


Similar to the test for $onInit on controllers, there seems to be two variables here: on the prototype or not and bind to controller or not. If these are independent then perhaps we need four checks?

Yes, that's right: We have two independent variables (bindToControler: true/false and prototype vs instance). Actually, we have a third one which is scope vs controller. So, ideally we need 8 different components tested per hook (× 5 hooks × 2 (dynamic + static upgrade) = 80 components). I cheated by testing two variables per component to save 50% of components tested.

In an ideal world, where bindToController is indeed independent from prototype vs instance, this should be OK. In the non-ideal world that we live in, we need the 40 extra components (or double as much faith 😛 )

petebacondarwin · 2016-12-19T09:40:25Z

modules/@angular/upgrade/test/upgrade_spec.ts

+               ref.dispose();
+             });
+           }));
+        it('should not call `$onDestroy()` on scope', fakeAsync(() => {


Missing carriage return here?

gkalpak · 2016-12-19T20:40:37Z

(I just added a missing newline to fix comment, hence the rebase.)

chuckjaz · 2016-12-20T22:56:48Z

@gkalpak Can you rebase this to the current master to resolve conflicts?

gkalpak · 2016-12-21T10:39:14Z

@chuckjaz, rebased.

I added one more fixup commit with the changes I made while resolving conflicts. Let me know if you prefer me to squash it.

petebacondarwin · 2016-12-21T11:13:29Z

@gkalpak the extra commit looks good. I think you should squashi it, so that it the merge is as straightforward as possible for the caretaker.

gkalpak · 2016-12-21T11:42:04Z

Done.

chuckjaz · 2016-12-21T18:28:24Z

modules/@angular/upgrade/src/upgrade_ng1_adapter.ts

@@ -313,6 +326,12 @@ class UpgradeNg1ComponentAdapter implements OnInit, OnChanges, DoCheck {
    return controller;
  }

+  private callLifecycleHook(method: LifecycleHook, context: IBindingDestination, arg?: any) {
+    if (context && typeof context[method] === 'function') {
+      context[method](arg);


This style of invocation breaks Closure advanced optimizations and we try to avoid this. Can you change this to avoid the dynamic invocation? I would prefer the code duplication than using dynamic indexing to avoid it.

@chuckjaz, can you give me some more context? Do the optimizations cause the resulting code to break or does the dynamic invocation prevent the optimizations from being applied?

I would prefer the code duplication than using dynamic indexing to avoid it.

Do you mean remove the callLifecycleHook method altogether and instead "inline" the code wherever the method was called?

So it is possible that inside a Closure compiled app the $onInit, for example, method might be renamed to something "smaller" like k. In this case this function would break the behaviour since it would still be trying to call $onInit.

We are not sure if this is the case, and it might be possible to get around it be annotating the function to prevent renaming.

But @chuckjaz is suggesting that it is safer simply to not do this in the first place.

chuckjaz · 2016-12-22T01:11:19Z

@gkalpak Can you fix the formatting break? Ensure the gulp lint is clean.

With the exception of `$onChanges()`, all lifecycle hooks in ng1 are called on the controller, regardless if it is the binding destination or not (i.e. regardless of the value of `bindToController`). This change makes `upgrade` mimic that behavior when calling lifecycle hooks. Additionally, calling the `$onInit()` hook has been moved before calling the linking functions, which also mimics the ng1 behavior.

gkalpak · 2016-12-22T09:47:16Z

@chuckjaz, I tend to forget about that 😞
Fixed now.

…mponent` Get rid of the dynamic invocation style used in `callLifecycleHook()`, which would break under Closure Compiler's advanced optimizations. Related to angular#13020 (comment).

With the exception of `$onChanges()`, all lifecycle hooks in ng1 are called on the controller, regardless if it is the binding destination or not (i.e. regardless of the value of `bindToController`). This change makes `upgrade` mimic that behavior when calling lifecycle hooks. Additionally, calling the `$onInit()` hook has been moved before calling the linking functions, which also mimics the ng1 behavior.

…mponent` (#13629) Get rid of the dynamic invocation style used in `callLifecycleHook()`, which would break under Closure Compiler's advanced optimizations. Related to #13020 (comment). PR Close #13629

…mponent` (angular#13629) Get rid of the dynamic invocation style used in `callLifecycleHook()`, which would break under Closure Compiler's advanced optimizations. Related to angular#13020 (comment). PR Close angular#13629

angular-automatic-lock-bot · 2019-09-10T13:20:53Z

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 22, 2016

gkalpak force-pushed the fix-ngUpgrade-jit-lifecycle-hooks branch from 0969a39 to 223870e Compare November 23, 2016 10:06

gkalpak changed the title ~~fix(UpgradeNg1Component): fix/improve support for lifecycle hooks~~ fix(upgrade): fix/improve support for lifecycle hooks Nov 23, 2016

vicb added comp: upgrade/dynamic action: review The PR is still awaiting reviews from at least one requested reviewer labels Dec 9, 2016

mgol reviewed Dec 14, 2016

View reviewed changes

mgol approved these changes Dec 14, 2016

View reviewed changes

petebacondarwin approved these changes Dec 19, 2016

View reviewed changes

petebacondarwin added action: merge The PR is ready for merge by the caretaker pr_state: LGTM and removed action: review The PR is still awaiting reviews from at least one requested reviewer labels Dec 19, 2016

gkalpak force-pushed the fix-ngUpgrade-jit-lifecycle-hooks branch from 223870e to 2c59924 Compare December 19, 2016 20:37

chuckjaz added action: cleanup The PR is in need of cleanup, either due to needing a rebase or in response to comments from reviews and removed action: merge The PR is ready for merge by the caretaker labels Dec 20, 2016

gkalpak force-pushed the fix-ngUpgrade-jit-lifecycle-hooks branch 3 times, most recently from 3e1a864 to 08d4c0f Compare December 21, 2016 10:33

gkalpak force-pushed the fix-ngUpgrade-jit-lifecycle-hooks branch from 08d4c0f to a320b66 Compare December 21, 2016 11:40

chuckjaz added the action: merge The PR is ready for merge by the caretaker label Dec 21, 2016

chuckjaz removed the action: cleanup The PR is in need of cleanup, either due to needing a rebase or in response to comments from reviews label Dec 21, 2016

chuckjaz suggested changes Dec 21, 2016

View reviewed changes

chuckjaz added action: cleanup The PR is in need of cleanup, either due to needing a rebase or in response to comments from reviews and removed action: merge The PR is ready for merge by the caretaker labels Dec 21, 2016

gkalpak force-pushed the fix-ngUpgrade-jit-lifecycle-hooks branch from 32afed2 to 0e34a9b Compare December 21, 2016 23:36

chuckjaz approved these changes Dec 22, 2016

View reviewed changes

gkalpak added 2 commits December 22, 2016 11:21

feat(upgrade): add support for the $postLink lifecycle hook

d67d90e

gkalpak force-pushed the fix-ngUpgrade-jit-lifecycle-hooks branch from 0e34a9b to d67d90e Compare December 22, 2016 09:29

gkalpak mentioned this pull request Dec 22, 2016

refactor(upgrade): allow Closure advanced optimizations in UpgradeComponent #13629

Closed

3 tasks

chuckjaz added action: merge The PR is ready for merge by the caretaker 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 Dec 22, 2016

hansl merged commit e5c4e58 into angular:master Dec 27, 2016

gkalpak deleted the fix-ngUpgrade-jit-lifecycle-hooks branch December 28, 2016 10:06

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(upgrade): fix/improve support for lifecycle hooks #13020

fix(upgrade): fix/improve support for lifecycle hooks #13020

gkalpak commented Nov 22, 2016

gkalpak commented Nov 22, 2016

chuckjaz commented Nov 22, 2016

gkalpak commented Nov 23, 2016

vicb commented Dec 9, 2016

gkalpak commented Dec 10, 2016

mgol Dec 14, 2016

gkalpak Dec 14, 2016

mgol Dec 14, 2016

mgol Dec 14, 2016

gkalpak Dec 14, 2016

mgol Dec 14, 2016

mgol Dec 14, 2016

petebacondarwin left a comment

petebacondarwin Dec 19, 2016

gkalpak Dec 19, 2016

petebacondarwin Dec 19, 2016

gkalpak Dec 19, 2016

petebacondarwin Dec 19, 2016

gkalpak commented Dec 19, 2016

chuckjaz commented Dec 20, 2016

gkalpak commented Dec 21, 2016

petebacondarwin commented Dec 21, 2016

gkalpak commented Dec 21, 2016

chuckjaz Dec 21, 2016

gkalpak Dec 21, 2016

petebacondarwin Dec 21, 2016

gkalpak Dec 21, 2016

chuckjaz commented Dec 22, 2016

gkalpak commented Dec 22, 2016

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

fix(upgrade): fix/improve support for lifecycle hooks #13020

fix(upgrade): fix/improve support for lifecycle hooks #13020

Conversation

gkalpak commented Nov 22, 2016

gkalpak commented Nov 22, 2016

chuckjaz commented Nov 22, 2016

gkalpak commented Nov 23, 2016

vicb commented Dec 9, 2016

gkalpak commented Dec 10, 2016

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

petebacondarwin 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

gkalpak commented Dec 19, 2016

chuckjaz commented Dec 20, 2016

gkalpak commented Dec 21, 2016

petebacondarwin commented Dec 21, 2016

gkalpak commented Dec 21, 2016

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

chuckjaz commented Dec 22, 2016

gkalpak commented Dec 22, 2016

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