fix(upgrade): properly destroy upgraded component elements and descendants #26209

jbedard · 2018-10-02T05:53:10Z

PR Type

[x] Bugfix

What is the current behavior?

Upgraded AngularJS dom elements are not destroyed properly (via jQuery) causing events+data to not be cleaned up. Child elements do not trigger the $destroy dom event.

Issue Number: #26208

What is the new behavior?

Upgraded AngularJS dom elements, along with descendants, have all events+data cleaned and $destroy triggered.

Does this PR introduce a breaking change?

[ ] Yes
[x] No

gkalpak

Nice catch 💯 I left a couple of comments, but overall LGTM 👍
IIRC this change also affects the dynamic version. If so, can you please add a test there as well?

gkalpak · 2018-10-02T10:16:54Z

packages/upgrade/src/common/angular1.ts

@@ -287,4 +287,7 @@ export const resumeBootstrap: typeof angular.resumeBootstrap = () => angular.res

 export const getTestability: typeof angular.getTestability = e => angular.getTestability(e);

+export const cleanData: (l: NodeList | Node[]) => void = (nodes) =>


Why expose it as angular.cleanData()? This is not consistent with the actual API 😕
We should put it on angular.element (and type it appropriately).

Put it on the element method exported in this file? I guess that would make it look more like the real thing being wrapped by this file...

Note that when doing import * as angular from this file it is already inconsistent with the actual API, it has things like setAngularJSGlobal on it etc. I was assuming it more like a bunch of helpers...

Having some extra APIs is one thing. Having the same helpers on different location (e.g. angular.cleanData() vs angular.element.cleanData()) is another story.

I agree that this file is already a little confusing. (I am not even sure why all the redirection tbh 😁)

Just to clarify, you mean changing this line to element.cleanData = ...?

This is what I had in mind:

let angular: { bootstrap: (e: Element, modules: (string | IInjectable)[], config?: IAngularBootstrapConfig) => IInjectorService, module: (prefix: string, dependencies?: string[]) => IModule, - element: (e: string | Element | Document | IAugmentedJQuery) => IAugmentedJQuery, + element: { + (e: string | Element | Document | IAugmentedJQuery): IAugmentedJQuery; + cleanData: (nodes: Node[] | NodeList[]) => void; + }, version: {major: number}, resumeBootstrap: () => void, getTestability: (e: Element) => ITestabilityService } = <any>{ bootstrap: noNg, module: noNg, - element: noNg, + element: Object.assign(() => noNg(), {cleanData: noNg}), version: undefined, resumeBootstrap: noNg, getTestability: noNg }; ... -export const element: typeof angular.element = e => angular.element(e); +export const element: typeof angular.element = Object.assign(e => angular.element(e), { + cleanData: nodes => angular.element.cleanData(nodes), +});

gkalpak · 2018-10-02T10:23:29Z

packages/upgrade/src/common/upgrade_helper.ts

@@ -124,7 +124,8 @@ export class UpgradeHelper {
      controllerInstance.$onDestroy();
    }
    $scope.$destroy();
-    this.$element.triggerHandler !('$destroy');
+    angular.cleanData([this.element]);
+    angular.cleanData(this.element.querySelectorAll('*'));


I suggest we implement a private jqLiteDealoc() helper and only call this.jqLiteDealoc(this.element) here.

The helper should follow the AngularJS implementation as close as possible (e.g. have a check similar to jsLiteAcceptsData() and if (element.querySelectorAll)), but it can omit features that we don't use (such as onlyDescendants).

Also, please add a link to the AngularJS implementation for reference.

Note that the jQuery acceptsData is slightly different :/

I think this needs to be compatible with both? Although don't we already know that this.element is an element? Do we really need to do acceptsData on it?

Actually... since we know it's an Element isn't the existing solution fine?

WDYT about leaving this as-is? Since we know it's an Element...

I'm tempted to just put a comment referencing the jqLite and jQuery versions though...

SGTM. Definitely add that comment though 😁

gkalpak · 2018-10-02T10:26:36Z

packages/upgrade/test/static/integration/upgrade_component_spec.ts

@@ -3694,6 +3694,206 @@ withEachNg1Version(() => {
           });
         }));

+      it('should emit `$destroy` on `$element` descendants', async(() => {


I think it is better to merge this with the above test.

jbedard · 2018-10-03T22:06:07Z

I've added a commit trying to add upgrade/dynamic tests but something is not right... it seems like it's not invoking the ng1 component controller. Anything obvious I'm missing? I mostly copied the tests from the upgrade/static tests...

gkalpak

The reason the tests fail is that in upgrade/dynamic the controller (under certain circumstances) can be instantiated before the directive's template has been compiled. Thus $element.contents() inside the controller constructor is not what you think it is 😱

(Not sure why this is that way, but it is probably better not to alter upgrade/dynamic's behavior so drastically at this point.)

You can use the controller's $onInit() hook instead of the constructor.

gkalpak · 2018-10-04T07:50:07Z

packages/upgrade/src/common/angular1.ts

+export const element: typeof angular.element = Object.assign(
+  (e: string | Element | Document | IAugmentedJQuery) => angular.element(e),
+  {
+    cleanData: (nodes: NodeList | Node[]) => angular.element.cleanData(nodes)


Nit: The order of types is different than in angular's type above (Node[] | NodeList vs NodeList | Node[]) 😁

gkalpak · 2018-10-04T07:52:25Z

packages/upgrade/test/dynamic/upgrade_spec.ts

@@ -6,12 +6,13 @@
 * found in the LICENSE file at https://angular.io/license
 */

-import {ChangeDetectorRef, Component, EventEmitter, Input, NO_ERRORS_SCHEMA, NgModule, NgModuleFactory, NgZone, OnChanges, OnDestroy, Output, SimpleChange, SimpleChanges, Testability, destroyPlatform, forwardRef} from '@angular/core';
+import {ChangeDetectorRef, Component, Directive, EventEmitter, Input, NO_ERRORS_SCHEMA, NgModule, NgModuleFactory, NgZone, OnChanges, OnDestroy, Output, SimpleChange, SimpleChanges, Testability, destroyPlatform, forwardRef} from '@angular/core';


gkalpak

LGTM

packages/upgrade/src/common/upgrade_helper.ts

gkalpak · 2018-10-05T10:17:55Z

packages/upgrade/test/dynamic/upgrade_spec.ts

-                   controller: function($element: angular.IAugmentedJQuery) {
-                     $element.on !('$destroy', elementDestroyListener);
+                   controller: class {
+                     constructor(private $element: angular.IAugmentedJQuery) {} $onInit() {


Nice job, clang-format 👌
🤢 🤢 🤢

…dants Fixes angular#26208

jbedard · 2018-10-05T18:19:34Z

An integration test is failing, twice in a row now. Seems like it is randomly not picking up key presses (picked up a different number of them each run). However that test doesn't use ng-upgrade from what I can see, so I assume it's unrelated...

gkalpak · 2018-10-06T09:10:59Z

@jbedard: You just need to keep pushing that button 😛 It's all green now.

Note to caretaker: Needs g3 sync.

jasonaden · 2018-10-08T18:00:41Z

Presubmit

…dants (#26209) Fixes #26208 PR Close #26209

…d descendants (angular#26209)" This reverts commit 5a31bde.

…d descendants (#26209)" This reverts commit 623adbb.

jasonaden · 2018-10-08T21:41:12Z

@gkalpak & @jbedard Please make an adjustment to this PR. I'm not sure why this wasn't caught earlier, but the build fails within Google due to the Object.assign calls. Please convert to destructuring or split onto multiple lines.

…d descendants (angular#26209)" This reverts commit 912f3d1. Revert is needed due to compilation failures due to this PR inside Google.

…d descendants (#26209)" This reverts commit 6da3867. Revert is needed due to compilation failures due to this PR inside Google.

jasonaden · 2018-10-10T21:53:22Z

Unfortunately this continues to fail in g3. Property assignment to function needs more type information on prior versions of TypeScript. Probably changing this to a union of typeof angular.element and {cleanData: noNg} is needed. Possibly a cast to any as well.

jasonaden · 2018-10-10T21:56:10Z

Internally in Google we get the same failure as on the 6.1.x branch: https://travis-ci.org/angular/angular/jobs/439854930

Again, due to a slightly older version of TypeScript.

jbedard · 2018-10-10T22:07:08Z

That's the problem I was having locally. I think if you take out the last commit by @gkalpak (f5447a3) it will work. I can fix it in the PR later today.

gkalpak · 2018-10-10T22:15:58Z

I'll fix it up 😞

… descendants

googlebot · 2018-10-10T22:24:04Z

So there's good news and bad news.

👍 The good news is that everyone that needs to sign a CLA (the pull request submitter and all commit authors) have done so. Everything is all good there.

😕 The bad news is that it appears that one or more commits were authored or co-authored by someone other than the pull request submitter. We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that here in the pull request.

Note to project maintainer: This is a terminal state, meaning the cla/google commit status will not change from this state. It's up to you to confirm consent of all the commit author(s), set the cla label to yes (if enabled on your project), and then merge this pull request when appropriate.

gkalpak · 2018-10-11T07:45:55Z

The CI failures are unrelated to this PR 😞

googlebot · 2018-10-11T19:46:49Z

A Googler has manually verified that the CLAs look good.

(Googler, please make sure the reason for overriding the CLA status is clearly documented in these comments.)

mhevery · 2018-10-11T21:15:03Z

http://test/OCL:216758713:BASE:216758772:1539292493195:9b1e806b

…dants (#26209) Fixes #26208 PR Close #26209

gkalpak · 2018-10-12T06:56:58Z

Fingers crossed it stays in this time 😁

…dants (angular#26209) Fixes angular#26208 PR Close angular#26209

…d descendants (angular#26209)" This reverts commit 912f3d1. Revert is needed due to compilation failures due to this PR inside Google.

…dants (angular#26209) Fixes angular#26208 PR Close angular#26209

…d descendants (angular#26209)" This reverts commit 5a31bde.

…dants (angular#26209) Fixes angular#26208 PR Close angular#26209

…d descendants (angular#26209)" This reverts commit 912f3d1. Revert is needed due to compilation failures due to this PR inside Google.

…dants (angular#26209) Fixes angular#26208 PR Close angular#26209

angular-automatic-lock-bot · 2019-09-14T03:53:10Z

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 Oct 2, 2018

jbedard force-pushed the upgraded-cleandata branch 2 times, most recently from 53267b4 to 528fe26 Compare October 2, 2018 06:28

gkalpak added type: bug/fix action: review The PR is still awaiting reviews from at least one requested reviewer comp: upgrade/static target: patch This PR is targeted for the next patch release labels Oct 2, 2018

gkalpak requested changes Oct 2, 2018

View reviewed changes

gkalpak reviewed Oct 4, 2018

View reviewed changes

jbedard force-pushed the upgraded-cleandata branch from d32c85c to f9b55a6 Compare October 5, 2018 08:20

gkalpak approved these changes Oct 5, 2018

View reviewed changes

gkalpak 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: review The PR is still awaiting reviews from at least one requested reviewer labels Oct 5, 2018

jbedard force-pushed the upgraded-cleandata branch from f9b55a6 to e383c49 Compare October 5, 2018 17:07

fix(upgrade): properly destroy upgraded component elements and descen…

10bf47e

…dants Fixes angular#26208

jbedard force-pushed the upgraded-cleandata branch from e383c49 to 10bf47e Compare October 5, 2018 17:42

jasonaden pushed a commit that referenced this pull request Oct 8, 2018

fix(upgrade): properly destroy upgraded component elements and descen…

623adbb

…dants (#26209) Fixes #26208 PR Close #26209

jasonaden closed this in 5a31bde Oct 8, 2018

jasonaden added a commit to jasonaden/angular that referenced this pull request Oct 8, 2018

Revert "fix(upgrade): properly destroy upgraded component elements an…

9623e7c

…d descendants (angular#26209)" This reverts commit 5a31bde.

jasonaden added a commit that referenced this pull request Oct 8, 2018

Revert "fix(upgrade): properly destroy upgraded component elements an…

b22c376

…d descendants (#26209)" This reverts commit 623adbb.

jasonaden reopened this Oct 8, 2018

jasonaden 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 Oct 8, 2018

jasonaden added a commit that referenced this pull request Oct 10, 2018

Revert "fix(upgrade): properly destroy upgraded component elements an…

65555fe

…d descendants (#26209)" This reverts commit 6da3867. Revert is needed due to compilation failures due to this PR inside Google.

jasonaden reopened this Oct 10, 2018

fixup! fix(upgrade): properly destroy upgraded component elements and…

3586fc6

… descendants

googlebot added cla: no and removed cla: yes labels Oct 10, 2018

mhevery added cla: yes and removed cla: no labels Oct 11, 2018

gkalpak approved these changes Oct 11, 2018

View reviewed changes

mhevery removed the merge: caretaker note Alert the caretaker performing the merge to check the PR for an out of normal action needed or note label Oct 11, 2018

mhevery pushed a commit that referenced this pull request Oct 12, 2018

fix(upgrade): properly destroy upgraded component elements and descen…

4f09f7d

…dants (#26209) Fixes #26208 PR Close #26209

mhevery closed this in 071934e Oct 12, 2018

wmarques pushed a commit to wmarques/angular that referenced this pull request Oct 14, 2018

fix(upgrade): properly destroy upgraded component elements and descen…

6c6bf5a

…dants (angular#26209) Fixes angular#26208 PR Close angular#26209

FrederikSchlemmer pushed a commit to FrederikSchlemmer/angular that referenced this pull request Jan 3, 2019

fix(upgrade): properly destroy upgraded component elements and descen…

bb0e2fa

…dants (angular#26209) Fixes angular#26208 PR Close angular#26209

FrederikSchlemmer pushed a commit to FrederikSchlemmer/angular that referenced this pull request Jan 3, 2019

Revert "fix(upgrade): properly destroy upgraded component elements an…

fe06fb9

…d descendants (angular#26209)" This reverts commit 5a31bde.

FrederikSchlemmer pushed a commit to FrederikSchlemmer/angular that referenced this pull request Jan 3, 2019

fix(upgrade): properly destroy upgraded component elements and descen…

50b7eb2

…dants (angular#26209) Fixes angular#26208 PR Close angular#26209

FrederikSchlemmer pushed a commit to FrederikSchlemmer/angular that referenced this pull request Jan 3, 2019

fix(upgrade): properly destroy upgraded component elements and descen…

004eb41

…dants (angular#26209) Fixes angular#26208 PR Close angular#26209

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(upgrade): properly destroy upgraded component elements and descendants #26209

fix(upgrade): properly destroy upgraded component elements and descendants #26209

jbedard commented Oct 2, 2018

gkalpak left a comment

gkalpak Oct 2, 2018

jbedard Oct 2, 2018

jbedard Oct 2, 2018

gkalpak Oct 2, 2018

jbedard Oct 2, 2018

gkalpak Oct 3, 2018

gkalpak Oct 2, 2018

jbedard Oct 2, 2018

jbedard Oct 2, 2018

jbedard Oct 3, 2018

gkalpak Oct 4, 2018

gkalpak Oct 2, 2018

jbedard commented Oct 3, 2018

gkalpak left a comment

gkalpak Oct 4, 2018

gkalpak Oct 4, 2018

gkalpak left a comment

gkalpak Oct 5, 2018

jbedard commented Oct 5, 2018

gkalpak commented Oct 6, 2018

jasonaden commented Oct 8, 2018

jasonaden commented Oct 8, 2018

jasonaden commented Oct 10, 2018

jasonaden commented Oct 10, 2018

jbedard commented Oct 10, 2018

gkalpak commented Oct 10, 2018

googlebot commented Oct 10, 2018

gkalpak commented Oct 11, 2018

googlebot commented Oct 11, 2018

mhevery commented Oct 11, 2018

gkalpak commented Oct 12, 2018

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

		@@ -287,4 +287,7 @@ export const resumeBootstrap: typeof angular.resumeBootstrap = () => angular.res

		export const getTestability: typeof angular.getTestability = e => angular.getTestability(e);

		export const cleanData: (l: NodeList \| Node[]) => void = (nodes) =>

fix(upgrade): properly destroy upgraded component elements and descendants #26209

fix(upgrade): properly destroy upgraded component elements and descendants #26209

Conversation

jbedard commented Oct 2, 2018

PR Type

What is the current behavior?

What is the new behavior?

Does this PR introduce a breaking change?

gkalpak 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

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

jbedard commented Oct 3, 2018

gkalpak left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gkalpak left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jbedard commented Oct 5, 2018

gkalpak commented Oct 6, 2018

jasonaden commented Oct 8, 2018

jasonaden commented Oct 8, 2018

jasonaden commented Oct 10, 2018

jasonaden commented Oct 10, 2018

jbedard commented Oct 10, 2018

gkalpak commented Oct 10, 2018

googlebot commented Oct 10, 2018

gkalpak commented Oct 11, 2018

googlebot commented Oct 11, 2018

mhevery commented Oct 11, 2018

gkalpak commented Oct 12, 2018

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