Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Closed
wants to merge 4 commits into from

Conversation

jbedard
Copy link
Contributor

@jbedard jbedard commented Oct 2, 2018

Fixes #26208

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

@jbedard jbedard force-pushed the upgraded-cleandata branch 2 times, most recently from 53267b4 to 528fe26 Compare Oct 2, 2018
@gkalpak gkalpak added type: bug/fix action: review comp: upgrade/static target: patch This PR is targeted for the next patch release labels Oct 2, 2018
Copy link
Member

@gkalpak gkalpak left a comment

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?

@@ -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) =>
Copy link
Member

@gkalpak gkalpak Oct 2, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

@jbedard jbedard Oct 2, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

@jbedard jbedard Oct 2, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

@gkalpak gkalpak Oct 2, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 😁)

Copy link
Contributor Author

@jbedard jbedard Oct 2, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

@gkalpak gkalpak Oct 3, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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),
+});

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

@gkalpak gkalpak Oct 2, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

@jbedard jbedard Oct 2, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

@jbedard jbedard Oct 2, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

@jbedard jbedard Oct 3, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

@gkalpak gkalpak Oct 4, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

SGTM. Definitely add that comment though 😁

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

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

@gkalpak gkalpak Oct 2, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@jbedard
Copy link
Contributor Author

@jbedard jbedard commented Oct 3, 2018

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

Copy link
Member

@gkalpak gkalpak left a comment

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.

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

@gkalpak gkalpak Oct 4, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@@ -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';
Copy link
Member

@gkalpak gkalpak Oct 4, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unused?

gkalpak
gkalpak approved these changes Oct 5, 2018
Copy link
Member

@gkalpak gkalpak left a comment

LGTM

packages/upgrade/src/common/upgrade_helper.ts Show resolved Hide resolved
controller: function($element: angular.IAugmentedJQuery) {
$element.on !('$destroy', elementDestroyListener);
controller: class {
constructor(private $element: angular.IAugmentedJQuery) {} $onInit() {
Copy link
Member

@gkalpak gkalpak Oct 5, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice job, clang-format 👌
🤢 🤢 🤢

@jbedard
Copy link
Contributor Author

@jbedard jbedard commented Oct 5, 2018

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 gkalpak added action: merge PR author is ready for this to merge action: merge-assistance CARETAKER: please help get this green and removed action: cleanup labels Oct 6, 2018
@gkalpak
Copy link
Member

@gkalpak gkalpak commented Oct 6, 2018

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

Note to caretaker: Needs g3 sync.

@jasonaden
Copy link
Contributor

@jasonaden jasonaden commented Oct 8, 2018

Presubmit

@jasonaden jasonaden closed this in 5a31bde Oct 8, 2018
jasonaden added a commit to jasonaden/angular that referenced this issue Oct 8, 2018
jasonaden added a commit that referenced this issue Oct 8, 2018
@jasonaden jasonaden reopened this Oct 8, 2018
@jasonaden
Copy link
Contributor

@jasonaden jasonaden commented Oct 8, 2018

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

jasonaden added a commit to jasonaden/angular that referenced this issue Oct 10, 2018
…d descendants (angular#26209)"

This reverts commit 912f3d1. Revert is needed due to compilation failures due to this PR inside Google.
jasonaden added a commit that referenced this issue Oct 10, 2018
…d descendants (#26209)"

This reverts commit 6da3867. Revert is needed due to compilation failures due to this PR inside Google.
@jasonaden jasonaden reopened this Oct 10, 2018
@jasonaden
Copy link
Contributor

@jasonaden jasonaden commented Oct 10, 2018

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
Copy link
Contributor

@jasonaden jasonaden commented Oct 10, 2018

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
Copy link
Contributor Author

@jbedard jbedard commented Oct 10, 2018

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
Copy link
Member

@gkalpak gkalpak commented Oct 10, 2018

I'll fix it up 😞

@googlebot
Copy link

@googlebot googlebot commented Oct 10, 2018

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
Copy link
Member

@gkalpak gkalpak commented Oct 11, 2018

The CI failures are unrelated to this PR 😞

@mhevery mhevery added cla: yes and removed cla: no labels Oct 11, 2018
@googlebot
Copy link

@googlebot googlebot commented Oct 11, 2018

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
Copy link
Contributor

@mhevery mhevery commented Oct 11, 2018

@mhevery mhevery removed the action: merge-assistance CARETAKER: please help get this green label Oct 11, 2018
@mhevery mhevery closed this in 071934e Oct 12, 2018
@gkalpak
Copy link
Member

@gkalpak gkalpak commented Oct 12, 2018

Fingers crossed it stays in this time 😁

wmarques pushed a commit to wmarques/angular that referenced this issue Oct 14, 2018
wmarques pushed a commit to wmarques/angular that referenced this issue Oct 14, 2018
…d descendants (angular#26209)"

This reverts commit 912f3d1. Revert is needed due to compilation failures due to this PR inside Google.
FrederikSchlemmer pushed a commit to FrederikSchlemmer/angular that referenced this issue Jan 3, 2019
FrederikSchlemmer pushed a commit to FrederikSchlemmer/angular that referenced this issue Jan 3, 2019
FrederikSchlemmer pushed a commit to FrederikSchlemmer/angular that referenced this issue Jan 3, 2019
FrederikSchlemmer pushed a commit to FrederikSchlemmer/angular that referenced this issue Jan 3, 2019
…d descendants (angular#26209)"

This reverts commit 912f3d1. Revert is needed due to compilation failures due to this PR inside Google.
FrederikSchlemmer pushed a commit to FrederikSchlemmer/angular that referenced this issue Jan 3, 2019
@angular-automatic-lock-bot
Copy link

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

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.

@angular-automatic-lock-bot angular-automatic-lock-bot bot locked and limited conversation to collaborators Sep 14, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
action: merge PR author is ready for this to merge cla: yes target: patch This PR is targeted for the next patch release type: bug/fix
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants