From edf3659949f84590dde118e9902bef1e77009cb0 Mon Sep 17 00:00:00 2001 From: Manu MA Date: Thu, 6 Dec 2018 22:19:35 +0100 Subject: [PATCH] Fix some angular (#16615) * fix(angular): platform types fixes #16535 * fix(angular): memory leak in lifecycle events fixes #16285 * fix ci * single core --- .circleci/config.yml | 8 ++--- angular/src/providers/angular-delegate.ts | 39 ++++++++++++++++------- angular/src/providers/platform.ts | 17 +++++----- angular/src/util/util.ts | 8 ++--- 4 files changed, 44 insertions(+), 28 deletions(-) diff --git a/.circleci/config.yml b/.circleci/config.yml index f279db9059..786b33c27e 100644 --- a/.circleci/config.yml +++ b/.circleci/config.yml @@ -60,12 +60,9 @@ jobs: working_directory: /tmp/workspace/core - save_cache: *save-cache-core - run: - command: npm run build # --max-workers 1 --debug + command: npm run build -- --max-workers 1 working_directory: /tmp/workspace/core - save_cache: *save-cache-core-stencil - - run: - command: sudo npm link - working_directory: /tmp/workspace/core - persist_to_workspace: root: /tmp/workspace paths: @@ -80,6 +77,9 @@ jobs: - run: command: npm install working_directory: /tmp/workspace/angular + - run: + command: sudo npm link + working_directory: /tmp/workspace/core - run: command: sudo npm link @ionic/core working_directory: /tmp/workspace/angular diff --git a/angular/src/providers/angular-delegate.ts b/angular/src/providers/angular-delegate.ts index 76a4972ce5..d65dfb4786 100644 --- a/angular/src/providers/angular-delegate.ts +++ b/angular/src/providers/angular-delegate.ts @@ -1,5 +1,5 @@ import { ApplicationRef, ComponentFactoryResolver, Injectable, InjectionToken, Injector, NgZone, ViewContainerRef } from '@angular/core'; -import { FrameworkDelegate, ViewLifecycle } from '@ionic/core'; +import { FrameworkDelegate } from '@ionic/core'; import { NavParams } from '../directives/navigation/nav-params'; @@ -24,6 +24,7 @@ export class AngularDelegate { export class AngularFrameworkDelegate implements FrameworkDelegate { private elRefMap = new WeakMap(); + private elEventsMap = new WeakMap void>(); constructor( private resolver: ComponentFactoryResolver, @@ -37,7 +38,8 @@ export class AngularFrameworkDelegate implements FrameworkDelegate { return new Promise(resolve => { this.zone.run(() => { const el = attachView( - this.resolver, this.injector, this.location, this.appRef, this.elRefMap, + this.resolver, this.injector, this.location, this.appRef, + this.elRefMap, this.elEventsMap, container, component, params, cssClasses ); resolve(el); @@ -52,6 +54,11 @@ export class AngularFrameworkDelegate implements FrameworkDelegate { if (componentRef) { componentRef.destroy(); this.elRefMap.delete(component); + const unbindEvents = this.elEventsMap.get(component); + if (unbindEvents) { + unbindEvents(); + this.elEventsMap.delete(component); + } } resolve(); }); @@ -65,6 +72,7 @@ export function attachView( location: ViewContainerRef | undefined, appRef: ApplicationRef, elRefMap: WeakMap, + elEventsMap: WeakMap void>, container: any, component: any, params: any, cssClasses: string[] | undefined ) { const factory = resolver.resolveComponentFactory(component); @@ -86,34 +94,41 @@ export function attachView( hostElement.classList.add(clazz); } } - bindLifecycleEvents(instance, hostElement); + const unbindEvents = bindLifecycleEvents(instance, hostElement); container.appendChild(hostElement); if (!location) { appRef.attachView(componentRef.hostView); } - componentRef.changeDetectorRef.reattach(); elRefMap.set(hostElement, componentRef); + elEventsMap.set(hostElement, unbindEvents); return hostElement; } const LIFECYCLES = [ - ViewLifecycle.WillEnter, - ViewLifecycle.DidEnter, - ViewLifecycle.WillLeave, - ViewLifecycle.DidLeave, - ViewLifecycle.WillUnload + 'ionViewWillEnter', + 'ionViewDidEnter', + 'ionViewWillLeave', + 'ionViewDidLeave', + 'ionViewWillUnload' ]; export function bindLifecycleEvents(instance: any, element: HTMLElement) { - LIFECYCLES.forEach(eventName => { - element.addEventListener(eventName, (ev: any) => { + const unregisters = LIFECYCLES.map(eventName => { + const handler = (ev: any) => { if (typeof instance[eventName] === 'function') { instance[eventName](ev.detail); } - }); + }; + element.addEventListener(eventName, handler); + return () => { + element.removeEventListener(eventName, handler); + }; }); + return () => { + unregisters.forEach(fn => fn()); + }; } const NavParamsToken = new InjectionToken('NavParamsToken'); diff --git a/angular/src/providers/platform.ts b/angular/src/providers/platform.ts index 564664bb00..2a67391b96 100644 --- a/angular/src/providers/platform.ts +++ b/angular/src/providers/platform.ts @@ -1,10 +1,11 @@ -import { EventEmitter, Injectable } from '@angular/core'; +import { Injectable } from '@angular/core'; import { BackButtonDetail, Platforms, getPlatforms, isPlatform } from '@ionic/core'; import { proxyEvent } from '../util/util'; +import { Subject, Subscription } from 'rxjs'; -export interface BackButtonEmitter extends EventEmitter { - subscribeWithPriority(priority: number, callback: () => Promise | void): void; +export interface BackButtonEmitter extends Subject { + subscribeWithPriority(priority: number, callback: () => Promise | void): Subscription; } @Injectable() @@ -15,7 +16,7 @@ export class Platform { /** * @hidden */ - backButton: BackButtonEmitter = new EventEmitter() as any; + backButton: BackButtonEmitter = new Subject() as any; /** * The pause event emits when the native platform puts the application @@ -23,25 +24,25 @@ export class Platform { * application. This event would emit when a Cordova app is put into * the background, however, it would not fire on a standard web browser. */ - pause = new EventEmitter(); + pause = new Subject(); /** * The resume event emits when the native platform pulls the application * out from the background. This event would emit when a Cordova app comes * out from the background, however, it would not fire on a standard web browser. */ - resume = new EventEmitter(); + resume = new Subject(); /** * The resize event emits when the browser window has changed dimensions. This * could be from a browser window being physically resized, or from a device * changing orientation. */ - resize = new EventEmitter(); + resize = new Subject(); constructor() { this.backButton.subscribeWithPriority = function(priority, callback) { - return this.subscribe((ev: BackButtonDetail) => { + return this.subscribe(ev => { ev.register(priority, callback); }); }; diff --git a/angular/src/util/util.ts b/angular/src/util/util.ts index d0c3a8332d..c0993dfc79 100644 --- a/angular/src/util/util.ts +++ b/angular/src/util/util.ts @@ -1,4 +1,5 @@ -import { ElementRef, EventEmitter } from '@angular/core'; +import { ElementRef } from '@angular/core'; +import { Subject } from 'rxjs'; export function inputs(instance: any, el: ElementRef, props: string[]) { props.forEach(propName => { @@ -8,13 +9,12 @@ export function inputs(instance: any, el: ElementRef, props: string[]) { }); } -export function proxyEvent(emitter: EventEmitter, el: EventTarget, eventName: string) { +export function proxyEvent(emitter: Subject, el: EventTarget, eventName: string) { el.addEventListener(eventName, (ev) => { - emitter.emit(ev ? (ev as any).detail as T : undefined); + emitter.next(ev ? (ev as any).detail as T : undefined); }); } - export function proxyMethod(ctrlName: string, methodName: string, ...args: any[]) { const controller = ensureElementInBody(ctrlName); return controller.componentOnReady()