From c1f5f663cbb0259c2ceaf846929a2d00e9481b3d Mon Sep 17 00:00:00 2001 From: George Kalpakas Date: Thu, 10 Dec 2020 19:13:31 +0200 Subject: [PATCH 1/5] refactor(upgrade): remove unused variables This commit removes a couple of unused variables. --- .../src/dynamic/src/upgrade_adapter.ts | 3 +- packages/upgrade/static/src/upgrade_module.ts | 175 +++++++++--------- 2 files changed, 87 insertions(+), 91 deletions(-) diff --git a/packages/upgrade/src/dynamic/src/upgrade_adapter.ts b/packages/upgrade/src/dynamic/src/upgrade_adapter.ts index bbb304e494c00..0008620b12ff1 100644 --- a/packages/upgrade/src/dynamic/src/upgrade_adapter.ts +++ b/packages/upgrade/src/dynamic/src/upgrade_adapter.ts @@ -505,7 +505,6 @@ export class UpgradeAdapter { const delayApplyExps: Function[] = []; let original$applyFn: Function; let rootScopePrototype: any; - let rootScope: IRootScopeService; const upgradeAdapter = this; const ng1Module = this.ng1Module = angularModule(this.idPrefix, modules); const platformRef = platformBrowserDynamic(); @@ -533,7 +532,7 @@ export class UpgradeAdapter { } else { throw new Error('Failed to find \'$apply\' on \'$rootScope\'!'); } - return rootScope = rootScopeDelegate; + return rootScopeDelegate; } ]); if (ng1Injector.has($$TESTABILITY)) { diff --git a/packages/upgrade/static/src/upgrade_module.ts b/packages/upgrade/static/src/upgrade_module.ts index 899d5ab59c4c0..b3e32bc9861c3 100644 --- a/packages/upgrade/static/src/upgrade_module.ts +++ b/packages/upgrade/static/src/upgrade_module.ts @@ -170,111 +170,108 @@ export class UpgradeModule { const INIT_MODULE_NAME = UPGRADE_MODULE_NAME + '.init'; // Create an ng1 module to bootstrap - const initModule = - angularModule(INIT_MODULE_NAME, []) + angularModule(INIT_MODULE_NAME, []) - .constant(UPGRADE_APP_TYPE_KEY, UpgradeAppType.Static) + .constant(UPGRADE_APP_TYPE_KEY, UpgradeAppType.Static) - .value(INJECTOR_KEY, this.injector) + .value(INJECTOR_KEY, this.injector) - .factory( - LAZY_MODULE_REF, - [INJECTOR_KEY, (injector: Injector) => ({injector} as LazyModuleRef)]) + .factory( + LAZY_MODULE_REF, [INJECTOR_KEY, (injector: Injector) => ({injector} as LazyModuleRef)]) - .config([ - $PROVIDE, $INJECTOR, - ($provide: IProvideService, $injector: IInjectorService) => { - if ($injector.has($$TESTABILITY)) { - $provide.decorator($$TESTABILITY, [ - $DELEGATE, - (testabilityDelegate: ITestabilityService) => { - const originalWhenStable: Function = testabilityDelegate.whenStable; - const injector = this.injector; - // Cannot use arrow function below because we need the context - const newWhenStable = function(callback: Function) { - originalWhenStable.call(testabilityDelegate, function() { - const ng2Testability: Testability = injector.get(Testability); - if (ng2Testability.isStable()) { - callback(); - } else { - ng2Testability.whenStable( - newWhenStable.bind(testabilityDelegate, callback)); - } - }); - }; + .config([ + $PROVIDE, $INJECTOR, + ($provide: IProvideService, $injector: IInjectorService) => { + if ($injector.has($$TESTABILITY)) { + $provide.decorator($$TESTABILITY, [ + $DELEGATE, + (testabilityDelegate: ITestabilityService) => { + const originalWhenStable: Function = testabilityDelegate.whenStable; + const injector = this.injector; + // Cannot use arrow function below because we need the context + const newWhenStable = function(callback: Function) { + originalWhenStable.call(testabilityDelegate, function() { + const ng2Testability: Testability = injector.get(Testability); + if (ng2Testability.isStable()) { + callback(); + } else { + ng2Testability.whenStable( + newWhenStable.bind(testabilityDelegate, callback)); + } + }); + }; - testabilityDelegate.whenStable = newWhenStable; - return testabilityDelegate; - } - ]); + testabilityDelegate.whenStable = newWhenStable; + return testabilityDelegate; } + ]); + } - if ($injector.has($INTERVAL)) { - $provide.decorator($INTERVAL, [ - $DELEGATE, - (intervalDelegate: IIntervalService) => { - // Wrap the $interval service so that setInterval is called outside NgZone, - // but the callback is still invoked within it. This is so that $interval - // won't block stability, which preserves the behavior from AngularJS. - let wrappedInterval = - (fn: Function, delay: number, count?: number, invokeApply?: boolean, - ...pass: any[]) => { - return this.ngZone.runOutsideAngular(() => { - return intervalDelegate((...args: any[]) => { - // Run callback in the next VM turn - $interval calls - // $rootScope.$apply, and running the callback in NgZone will - // cause a '$digest already in progress' error if it's in the - // same vm turn. - setTimeout(() => { - this.ngZone.run(() => fn(...args)); - }); - }, delay, count, invokeApply, ...pass); + if ($injector.has($INTERVAL)) { + $provide.decorator($INTERVAL, [ + $DELEGATE, + (intervalDelegate: IIntervalService) => { + // Wrap the $interval service so that setInterval is called outside NgZone, + // but the callback is still invoked within it. This is so that $interval + // won't block stability, which preserves the behavior from AngularJS. + let wrappedInterval = + (fn: Function, delay: number, count?: number, invokeApply?: boolean, + ...pass: any[]) => { + return this.ngZone.runOutsideAngular(() => { + return intervalDelegate((...args: any[]) => { + // Run callback in the next VM turn - $interval calls + // $rootScope.$apply, and running the callback in NgZone will + // cause a '$digest already in progress' error if it's in the + // same vm turn. + setTimeout(() => { + this.ngZone.run(() => fn(...args)); }); - }; + }, delay, count, invokeApply, ...pass); + }); + }; - (wrappedInterval as any)['cancel'] = intervalDelegate.cancel; - return wrappedInterval; - } - ]); + (wrappedInterval as any)['cancel'] = intervalDelegate.cancel; + return wrappedInterval; } - } - ]) + ]); + } + } + ]) - .run([ - $INJECTOR, - ($injector: IInjectorService) => { - this.$injector = $injector; + .run([ + $INJECTOR, + ($injector: IInjectorService) => { + this.$injector = $injector; - // Initialize the ng1 $injector provider - setTempInjectorRef($injector); - this.injector.get($INJECTOR); + // Initialize the ng1 $injector provider + setTempInjectorRef($injector); + this.injector.get($INJECTOR); - // Put the injector on the DOM, so that it can be "required" - angularElement(element).data!(controllerKey(INJECTOR_KEY), this.injector); + // Put the injector on the DOM, so that it can be "required" + angularElement(element).data!(controllerKey(INJECTOR_KEY), this.injector); - // Wire up the ng1 rootScope to run a digest cycle whenever the zone settles - // We need to do this in the next tick so that we don't prevent the bootup - // stabilizing - setTimeout(() => { - const $rootScope = $injector.get('$rootScope'); - const subscription = this.ngZone.onMicrotaskEmpty.subscribe(() => { - if ($rootScope.$$phase) { - if (isDevMode()) { - console.warn( - 'A digest was triggered while one was already in progress. This may mean that something is triggering digests outside the Angular zone.'); - } + // Wire up the ng1 rootScope to run a digest cycle whenever the zone settles + // We need to do this in the next tick so that we don't prevent the bootup stabilizing + setTimeout(() => { + const $rootScope = $injector.get('$rootScope'); + const subscription = this.ngZone.onMicrotaskEmpty.subscribe(() => { + if ($rootScope.$$phase) { + if (isDevMode()) { + console.warn( + 'A digest was triggered while one was already in progress. This may mean that something is triggering digests outside the Angular zone.'); + } - return $rootScope.$evalAsync(); - } + return $rootScope.$evalAsync(); + } - return $rootScope.$digest(); - }); - $rootScope.$on('$destroy', () => { - subscription.unsubscribe(); - }); - }, 0); - } - ]); + return $rootScope.$digest(); + }); + $rootScope.$on('$destroy', () => { + subscription.unsubscribe(); + }); + }, 0); + } + ]); const upgradeModule = angularModule(UPGRADE_MODULE_NAME, [INIT_MODULE_NAME].concat(modules)); From 8ff6319f5d21f00f8412899442c87b81bd75eea0 Mon Sep 17 00:00:00 2001 From: George Kalpakas Date: Thu, 10 Dec 2020 19:13:32 +0200 Subject: [PATCH 2/5] refactor(upgrade): create a helper for cleaning jqLite/jQuery data This commit moves the code for cleaning jqLite/jQuery data on an element to a re-usable helper function. This way it is easier to keep the code consistent across all places where we need to clean data (now and in the future). --- .../common/src/downgrade_component_adapter.ts | 11 +++------- .../upgrade/src/common/src/upgrade_helper.ts | 12 ++--------- packages/upgrade/src/common/src/util.ts | 21 ++++++++++++++++++- 3 files changed, 25 insertions(+), 19 deletions(-) diff --git a/packages/upgrade/src/common/src/downgrade_component_adapter.ts b/packages/upgrade/src/common/src/downgrade_component_adapter.ts index 5022a58aaca22..8711624f3bad5 100644 --- a/packages/upgrade/src/common/src/downgrade_component_adapter.ts +++ b/packages/upgrade/src/common/src/downgrade_component_adapter.ts @@ -8,10 +8,10 @@ import {ApplicationRef, ChangeDetectorRef, ComponentFactory, ComponentRef, EventEmitter, Injector, OnChanges, SimpleChange, SimpleChanges, StaticProvider, Testability, TestabilityRegistry, Type} from '@angular/core'; -import {element as angularElement, IAttributes, IAugmentedJQuery, ICompileService, INgModelController, IParseService, IScope} from './angular1'; +import {IAttributes, IAugmentedJQuery, ICompileService, INgModelController, IParseService, IScope} from './angular1'; import {PropertyBinding} from './component_info'; import {$SCOPE} from './constants'; -import {getTypeName, hookupNgModel, strictEquals} from './util'; +import {cleanData, getTypeName, hookupNgModel, strictEquals} from './util'; const INITIAL_VALUE = { __UNINITIALIZED__: true @@ -241,12 +241,7 @@ export class DowngradeComponentAdapter { // // To ensure the element is always properly cleaned up, we manually call `cleanData()` on // this element and its descendants before destroying the `ComponentRef`. - // - // NOTE: - // `cleanData()` also will invoke the AngularJS `$destroy` event on the element: - // https://github.com/angular/angular.js/blob/2e72ea13fa98bebf6ed4b5e3c45eaf5f990ed16f/src/Angular.js#L1932-L1945 - angularElement.cleanData(this.element); - angularElement.cleanData((this.element[0] as Element).querySelectorAll('*')); + cleanData(this.element[0]); destroyComponentRef(); } diff --git a/packages/upgrade/src/common/src/upgrade_helper.ts b/packages/upgrade/src/common/src/upgrade_helper.ts index 0d520eb9e6a4b..71a0c56f9233c 100644 --- a/packages/upgrade/src/common/src/upgrade_helper.ts +++ b/packages/upgrade/src/common/src/upgrade_helper.ts @@ -10,7 +10,7 @@ import {ElementRef, Injector, SimpleChanges} from '@angular/core'; import {DirectiveRequireProperty, element as angularElement, IAugmentedJQuery, ICloneAttachFunction, ICompileService, IController, IControllerService, IDirective, IHttpBackendService, IInjectorService, ILinkFn, IScope, ITemplateCacheService, SingleOrListOrMap} from './angular1'; import {$COMPILE, $CONTROLLER, $HTTP_BACKEND, $INJECTOR, $TEMPLATE_CACHE} from './constants'; -import {controllerKey, directiveNormalize, isFunction} from './util'; +import {cleanData, controllerKey, directiveNormalize, isFunction} from './util'; @@ -125,15 +125,7 @@ export class UpgradeHelper { controllerInstance.$onDestroy(); } $scope.$destroy(); - - // Clean the jQuery/jqLite data on the component+child elements. - // Equivelent to how jQuery/jqLite invoke `cleanData` on an Element (this.element) - // https://github.com/jquery/jquery/blob/e743cbd28553267f955f71ea7248377915613fd9/src/manipulation.js#L223 - // https://github.com/angular/angular.js/blob/26ddc5f830f902a3d22f4b2aab70d86d4d688c82/src/jqLite.js#L306-L312 - // `cleanData` will invoke the AngularJS `$destroy` DOM event - // https://github.com/angular/angular.js/blob/26ddc5f830f902a3d22f4b2aab70d86d4d688c82/src/Angular.js#L1911-L1924 - angularElement.cleanData([this.element]); - angularElement.cleanData(this.element.querySelectorAll('*')); + cleanData(this.element); } prepareTransclusion(): ILinkFn|undefined { diff --git a/packages/upgrade/src/common/src/util.ts b/packages/upgrade/src/common/src/util.ts index 1c772f9bb4213..5dbee8cb834ad 100644 --- a/packages/upgrade/src/common/src/util.ts +++ b/packages/upgrade/src/common/src/util.ts @@ -8,7 +8,7 @@ import {Injector, Type} from '@angular/core'; -import {IInjectorService, INgModelController} from './angular1'; +import {element as angularElement, IInjectorService, INgModelController} from './angular1'; import {DOWNGRADED_MODULE_COUNT_KEY, UPGRADE_APP_TYPE_KEY} from './constants'; const DIRECTIVE_PREFIX_REGEXP = /^(?:x|data)[:\-_]/i; @@ -25,6 +25,21 @@ export function onError(e: any) { throw e; } +export function cleanData(node: Node): void { + // Clean the jqLite/jQuery data on the element and all its descendants. + // Equivalent to how jqLite/jQuery invoke `cleanData()` on an Element when removed: + // https://github.com/angular/angular.js/blob/2e72ea13fa98bebf6ed4b5e3c45eaf5f990ed16f/src/jqLite.js#L349-L355 + // https://github.com/jquery/jquery/blob/6984d1747623dbc5e87fd6c261a5b6b1628c107c/src/manipulation.js#L182 + // + // NOTE: + // `cleanData()` will also invoke the AngularJS `$destroy` DOM event on the element: + // https://github.com/angular/angular.js/blob/2e72ea13fa98bebf6ed4b5e3c45eaf5f990ed16f/src/Angular.js#L1932-L1945 + angularElement.cleanData([node]); + if (isParentNode(node)) { + angularElement.cleanData(node.querySelectorAll('*')); + } +} + export function controllerKey(name: string): string { return '$' + name + 'Controller'; } @@ -53,6 +68,10 @@ export function isFunction(value: any): value is Function { return typeof value === 'function'; } +function isParentNode(node: Node|ParentNode): node is ParentNode { + return isFunction((node as unknown as ParentNode).querySelectorAll); +} + export function validateInjectionKey( $injector: IInjectorService, downgradedModule: string, injectionKey: string, attemptedAction: string): void { From e80c8f5dc0f5a00fc7d0377719ca63034ccfdb54 Mon Sep 17 00:00:00 2001 From: George Kalpakas Date: Thu, 10 Dec 2020 19:13:32 +0200 Subject: [PATCH 3/5] fixup! refactor(upgrade): create a helper for cleaning jqLite/jQuery data --- packages/upgrade/src/common/src/util.ts | 20 ++++++++++++-------- 1 file changed, 12 insertions(+), 8 deletions(-) diff --git a/packages/upgrade/src/common/src/util.ts b/packages/upgrade/src/common/src/util.ts index 5dbee8cb834ad..bb93e23a637ff 100644 --- a/packages/upgrade/src/common/src/util.ts +++ b/packages/upgrade/src/common/src/util.ts @@ -25,15 +25,19 @@ export function onError(e: any) { throw e; } +/** + * Clean the jqLite/jQuery data on the element and all its descendants. + * Equivalent to how jqLite/jQuery invoke `cleanData()` on an Element when removed: + * https://github.com/angular/angular.js/blob/2e72ea13fa98bebf6ed4b5e3c45eaf5f990ed16f/src/jqLite.js#L349-L355 + * https://github.com/jquery/jquery/blob/6984d1747623dbc5e87fd6c261a5b6b1628c107c/src/manipulation.js#L182 + * + * NOTE: + * `cleanData()` will also invoke the AngularJS `$destroy` DOM event on the element: + * https://github.com/angular/angular.js/blob/2e72ea13fa98bebf6ed4b5e3c45eaf5f990ed16f/src/Angular.js#L1932-L1945 + * + * @param node The DOM node whose data needs to be cleaned. + */ export function cleanData(node: Node): void { - // Clean the jqLite/jQuery data on the element and all its descendants. - // Equivalent to how jqLite/jQuery invoke `cleanData()` on an Element when removed: - // https://github.com/angular/angular.js/blob/2e72ea13fa98bebf6ed4b5e3c45eaf5f990ed16f/src/jqLite.js#L349-L355 - // https://github.com/jquery/jquery/blob/6984d1747623dbc5e87fd6c261a5b6b1628c107c/src/manipulation.js#L182 - // - // NOTE: - // `cleanData()` will also invoke the AngularJS `$destroy` DOM event on the element: - // https://github.com/angular/angular.js/blob/2e72ea13fa98bebf6ed4b5e3c45eaf5f990ed16f/src/Angular.js#L1932-L1945 angularElement.cleanData([node]); if (isParentNode(node)) { angularElement.cleanData(node.querySelectorAll('*')); From 43e994d5ec66dd0c5897d6433fab1c19e62ab3d5 Mon Sep 17 00:00:00 2001 From: George Kalpakas Date: Thu, 10 Dec 2020 19:13:33 +0200 Subject: [PATCH 4/5] fix(upgrade): fix HMR for hybrid applications Previously, trying to apply a change via Hot Module Replacement (HMR) in a hybrid app would result in an error. This was caused by not having the AngularJS app destroyed and thus trying to bootstrap an AngularJS app on the same element twice. This commit fixes HMR for hybrid apps by ensuring the AngularJS app is destroyed when the Angular `PlatformRef` is [destroyed][1] in the [`module.hot.dispose()` callback][2]. NOTE: For "ngUpgradeLite" apps (i.e. those using `downgradeModule()`), HMR will only work if the downgraded module has been bootstrapped and there is at least one Angular component present on the page. The is due to a combination of two facts: - The logic for setting up the listener that destroys the AngularJS app depends on the downgraded module's `NgModuleRef`, which is only available after the module has been bootstrapped. - The [HMR dispose logic][3] depends on having an Angular element (identified by the auto-geenrated `ng-version` attribute) present in the DOM in order to retrieve the Angular `PlatformRef`. [1]: https://github.com/angular/angular-cli/blob/205ea2b638f154291993bfd9e065cd66ff20503/packages/angular_devkit/build_angular/src/webpack/plugins/hmr/hmr-accept.ts#L75 [2]: https://github.com/angular/angular-cli/blob/205ea2b638f154291993bfd9e065cd66ff205033/packages/angular_devkit/build_angular/src/webpack/plugins/hmr/hmr-accept.ts#L31 [3]: https://github.com/angular/angular-cli/blob/205ea2b638f154291993bfd9e065cd66ff205033/packages/angular_devkit/build_angular/src/webpack/plugins/hmr/hmr-accept.ts#L116 Fixes #39935 --- goldens/public-api/upgrade/static/static.d.ts | 3 +- packages/upgrade/src/common/src/constants.ts | 1 + packages/upgrade/src/common/src/util.ts | 15 ++++- .../src/dynamic/src/upgrade_adapter.ts | 8 ++- .../upgrade/src/dynamic/test/upgrade_spec.ts | 60 +++++++++++++++++- .../upgrade/static/src/downgrade_module.ts | 10 ++- packages/upgrade/static/src/upgrade_module.ts | 20 ++++-- .../integration/downgrade_component_spec.ts | 61 ++++++++++++++++++ .../test/integration/downgrade_module_spec.ts | 62 +++++++++++++++++++ 9 files changed, 229 insertions(+), 11 deletions(-) diff --git a/goldens/public-api/upgrade/static/static.d.ts b/goldens/public-api/upgrade/static/static.d.ts index 47b7425ce9258..d21bc57848499 100644 --- a/goldens/public-api/upgrade/static/static.d.ts +++ b/goldens/public-api/upgrade/static/static.d.ts @@ -35,7 +35,8 @@ export declare class UpgradeModule { ngZone: NgZone; constructor( injector: Injector, - ngZone: NgZone); + ngZone: NgZone, + platformRef: PlatformRef); bootstrap(element: Element, modules?: string[], config?: any): void; } diff --git a/packages/upgrade/src/common/src/constants.ts b/packages/upgrade/src/common/src/constants.ts index 7c9c5d0c8f766..640d23adf0ea0 100644 --- a/packages/upgrade/src/common/src/constants.ts +++ b/packages/upgrade/src/common/src/constants.ts @@ -15,6 +15,7 @@ export const $INJECTOR = '$injector'; export const $INTERVAL = '$interval'; export const $PARSE = '$parse'; export const $PROVIDE = '$provide'; +export const $ROOT_ELEMENT = '$rootElement'; export const $ROOT_SCOPE = '$rootScope'; export const $SCOPE = '$scope'; export const $TEMPLATE_CACHE = '$templateCache'; diff --git a/packages/upgrade/src/common/src/util.ts b/packages/upgrade/src/common/src/util.ts index bb93e23a637ff..ebb81c311c54f 100644 --- a/packages/upgrade/src/common/src/util.ts +++ b/packages/upgrade/src/common/src/util.ts @@ -8,8 +8,8 @@ import {Injector, Type} from '@angular/core'; -import {element as angularElement, IInjectorService, INgModelController} from './angular1'; -import {DOWNGRADED_MODULE_COUNT_KEY, UPGRADE_APP_TYPE_KEY} from './constants'; +import {element as angularElement, IAugmentedJQuery, IInjectorService, INgModelController, IRootScopeService} from './angular1'; +import {$ROOT_ELEMENT, $ROOT_SCOPE, DOWNGRADED_MODULE_COUNT_KEY, UPGRADE_APP_TYPE_KEY} from './constants'; const DIRECTIVE_PREFIX_REGEXP = /^(?:x|data)[:\-_]/i; const DIRECTIVE_SPECIAL_CHARS_REGEXP = /[:\-_]+(.)/g; @@ -48,6 +48,17 @@ export function controllerKey(name: string): string { return '$' + name + 'Controller'; } +// Destroy an AngularJS app given the app `$injector`. +// +// NOTE: Destroying an app is not officially supported by AngularJS, but we do our best. +export function destroyApp($injector: IInjectorService): void { + const $rootElement: IAugmentedJQuery = $injector.get($ROOT_ELEMENT); + const $rootScope: IRootScopeService = $injector.get($ROOT_SCOPE); + + $rootScope.$destroy(); + cleanData($rootElement[0]); +} + export function directiveNormalize(name: string): string { return name.replace(DIRECTIVE_PREFIX_REGEXP, '') .replace(DIRECTIVE_SPECIAL_CHARS_REGEXP, (_, letter) => letter.toUpperCase()); diff --git a/packages/upgrade/src/dynamic/src/upgrade_adapter.ts b/packages/upgrade/src/dynamic/src/upgrade_adapter.ts index 0008620b12ff1..bf5edf0f2d685 100644 --- a/packages/upgrade/src/dynamic/src/upgrade_adapter.ts +++ b/packages/upgrade/src/dynamic/src/upgrade_adapter.ts @@ -13,7 +13,7 @@ import {bootstrap, element as angularElement, IAngularBootstrapConfig, IAugmente import {$$TESTABILITY, $COMPILE, $INJECTOR, $ROOT_SCOPE, COMPILER_KEY, INJECTOR_KEY, LAZY_MODULE_REF, NG_ZONE_KEY, UPGRADE_APP_TYPE_KEY} from '../../common/src/constants'; import {downgradeComponent} from '../../common/src/downgrade_component'; import {downgradeInjectable} from '../../common/src/downgrade_injectable'; -import {controllerKey, Deferred, LazyModuleRef, onError, UpgradeAppType} from '../../common/src/util'; +import {controllerKey, Deferred, destroyApp, LazyModuleRef, onError, UpgradeAppType} from '../../common/src/util'; import {UpgradeNg1ComponentAdapterBuilder} from './upgrade_ng1_adapter'; @@ -619,6 +619,12 @@ export class UpgradeAdapter { rootScope.$on('$destroy', () => { subscription.unsubscribe(); }); + + // Destroy the AngularJS app once the Angular `PlatformRef` is destroyed. + // This does not happen in a typical SPA scenario, but it might be useful for + // other usecases where desposing of an Angular/AngularJS app is necessary (such + // as Hot Module Replacement (HMR)). + platformRef.onDestroy(() => destroyApp(ng1Injector)); }); }) .catch((e) => this.ng2BootstrapDeferred.reject(e)); diff --git a/packages/upgrade/src/dynamic/test/upgrade_spec.ts b/packages/upgrade/src/dynamic/test/upgrade_spec.ts index 3d33cc59be02d..70bbbc66e38f9 100644 --- a/packages/upgrade/src/dynamic/test/upgrade_spec.ts +++ b/packages/upgrade/src/dynamic/test/upgrade_spec.ts @@ -86,7 +86,7 @@ withEachNg1Version(() => { }); })); - it('supports the compilerOptions argument', waitForAsync(() => { + it('should support the compilerOptions argument', waitForAsync(() => { const platformRef = platformBrowserDynamic(); spyOn(platformRef, 'bootstrapModule').and.callThrough(); spyOn(platformRef, 'bootstrapModuleFactory').and.callThrough(); @@ -120,6 +120,64 @@ withEachNg1Version(() => { ref.dispose(); }); })); + + it('should destroy the AngularJS app when `PlatformRef` is destroyed', waitForAsync(() => { + const platformRef = platformBrowserDynamic(); + const adapter = new UpgradeAdapter(forwardRef(() => Ng2Module)); + const ng1Module = angular.module_('ng1', []); + + @Component({selector: 'ng2', template: 'NG2'}) + class Ng2Component { + } + + @NgModule({ + declarations: [Ng2Component], + imports: [BrowserModule], + }) + class Ng2Module { + ngDoBootstrap() {} + } + + ng1Module.component('ng1', {template: ''}); + ng1Module.directive('ng2', adapter.downgradeNg2Component(Ng2Component)); + + const element = html('
'); + + adapter.bootstrap(element, [ng1Module.name]).ready(ref => { + const $rootScope: angular.IRootScopeService = ref.ng1Injector.get($ROOT_SCOPE); + const rootScopeDestroySpy = spyOn($rootScope, '$destroy'); + + const appElem = angular.element(element); + const ng1Elem = angular.element(element.querySelector('ng1') as Element); + const ng2Elem = angular.element(element.querySelector('ng2') as Element); + const ng2ChildElem = angular.element(element.querySelector('ng2 span') as Element); + + // Attach data to all elements. + appElem.data!('testData', 1); + ng1Elem.data!('testData', 2); + ng2Elem.data!('testData', 3); + ng2ChildElem.data!('testData', 4); + + // Verify data can be retrieved. + expect(appElem.data!('testData')).toBe(1); + expect(ng1Elem.data!('testData')).toBe(2); + expect(ng2Elem.data!('testData')).toBe(3); + expect(ng2ChildElem.data!('testData')).toBe(4); + + expect(rootScopeDestroySpy).not.toHaveBeenCalled(); + + // Destroy `PlatformRef`. + platformRef.destroy(); + + // Verify `$rootScope` has been destroyed and data has been cleaned up. + expect(rootScopeDestroySpy).toHaveBeenCalled(); + + expect(appElem.data!('testData')).toBeUndefined(); + expect(ng1Elem.data!('testData')).toBeUndefined(); + expect(ng2Elem.data!('testData')).toBeUndefined(); + expect(ng2ChildElem.data!('testData')).toBeUndefined(); + }); + })); }); describe('bootstrap errors', () => { diff --git a/packages/upgrade/static/src/downgrade_module.ts b/packages/upgrade/static/src/downgrade_module.ts index bde2fe8db93c2..070fd23d7e4fd 100644 --- a/packages/upgrade/static/src/downgrade_module.ts +++ b/packages/upgrade/static/src/downgrade_module.ts @@ -6,12 +6,12 @@ * found in the LICENSE file at https://angular.io/license */ -import {Injector, NgModuleFactory, NgModuleRef, StaticProvider} from '@angular/core'; +import {Injector, NgModuleFactory, NgModuleRef, PlatformRef, StaticProvider} from '@angular/core'; import {platformBrowser} from '@angular/platform-browser'; import {IInjectorService, IProvideService, module_ as angularModule} from '../../src/common/src/angular1'; import {$INJECTOR, $PROVIDE, DOWNGRADED_MODULE_COUNT_KEY, INJECTOR_KEY, LAZY_MODULE_REF, UPGRADE_APP_TYPE_KEY, UPGRADE_MODULE_NAME} from '../../src/common/src/constants'; -import {getDowngradedModuleCount, isFunction, LazyModuleRef, UpgradeAppType} from '../../src/common/src/util'; +import {destroyApp, getDowngradedModuleCount, isFunction, LazyModuleRef, UpgradeAppType} from '../../src/common/src/util'; import {angular1Providers, setTempInjectorRef} from './angular1_providers'; import {NgAdapterInjector} from './util'; @@ -167,6 +167,12 @@ export function downgradeModule(moduleFactoryOrBootstrapFn: NgModuleFactory destroyApp($injector)); + return injector; }) }; diff --git a/packages/upgrade/static/src/upgrade_module.ts b/packages/upgrade/static/src/upgrade_module.ts index b3e32bc9861c3..2ae8df10a50a5 100644 --- a/packages/upgrade/static/src/upgrade_module.ts +++ b/packages/upgrade/static/src/upgrade_module.ts @@ -6,11 +6,11 @@ * found in the LICENSE file at https://angular.io/license */ -import {Injector, isDevMode, NgModule, NgZone, Testability} from '@angular/core'; +import {Injector, isDevMode, NgModule, NgZone, PlatformRef, Testability} from '@angular/core'; import {bootstrap, element as angularElement, IInjectorService, IIntervalService, IProvideService, ITestabilityService, module_ as angularModule} from '../../src/common/src/angular1'; import {$$TESTABILITY, $DELEGATE, $INJECTOR, $INTERVAL, $PROVIDE, INJECTOR_KEY, LAZY_MODULE_REF, UPGRADE_APP_TYPE_KEY, UPGRADE_MODULE_NAME} from '../../src/common/src/constants'; -import {controllerKey, LazyModuleRef, UpgradeAppType} from '../../src/common/src/util'; +import {controllerKey, destroyApp, LazyModuleRef, UpgradeAppType} from '../../src/common/src/util'; import {angular1Providers, setTempInjectorRef} from './angular1_providers'; import {NgAdapterInjector} from './util'; @@ -155,7 +155,13 @@ export class UpgradeModule { /** The root `Injector` for the upgrade application. */ injector: Injector, /** The bootstrap zone for the upgrade application */ - public ngZone: NgZone) { + public ngZone: NgZone, + /** + * The owning `NgModuleRef`s `PlatformRef` instance. + * This is used to tie the lifecycle of the bootstrapped AngularJS apps to that of the Angular + * `PlatformRef`. + */ + private platformRef: PlatformRef) { this.injector = new NgAdapterInjector(injector); } @@ -242,6 +248,7 @@ export class UpgradeModule { $INJECTOR, ($injector: IInjectorService) => { this.$injector = $injector; + const $rootScope = $injector.get('$rootScope'); // Initialize the ng1 $injector provider setTempInjectorRef($injector); @@ -250,10 +257,15 @@ export class UpgradeModule { // Put the injector on the DOM, so that it can be "required" angularElement(element).data!(controllerKey(INJECTOR_KEY), this.injector); + // Destroy the AngularJS app once the Angular `PlatformRef` is destroyed. + // This does not happen in a typical SPA scenario, but it might be useful for + // other usecases where desposing of an Angular/AngularJS app is necessary (such + // as Hot Module Replacement (HMR)). + this.platformRef.onDestroy(() => destroyApp($injector)); + // Wire up the ng1 rootScope to run a digest cycle whenever the zone settles // We need to do this in the next tick so that we don't prevent the bootup stabilizing setTimeout(() => { - const $rootScope = $injector.get('$rootScope'); const subscription = this.ngZone.onMicrotaskEmpty.subscribe(() => { if ($rootScope.$$phase) { if (isDevMode()) { diff --git a/packages/upgrade/static/test/integration/downgrade_component_spec.ts b/packages/upgrade/static/test/integration/downgrade_component_spec.ts index ec88d423d158a..5076831f8f9b6 100644 --- a/packages/upgrade/static/test/integration/downgrade_component_spec.ts +++ b/packages/upgrade/static/test/integration/downgrade_component_spec.ts @@ -13,6 +13,7 @@ import {platformBrowserDynamic} from '@angular/platform-browser-dynamic'; import {downgradeComponent, UpgradeComponent, UpgradeModule} from '@angular/upgrade/static'; import * as angular from '../../../src/common/src/angular1'; +import {$ROOT_SCOPE} from '../../../src/common/src/constants'; import {html, multiTrim, withEachNg1Version} from '../../../src/common/test/helpers/common_test_helpers'; import {$apply, bootstrap} from './static_test_helpers'; @@ -648,6 +649,66 @@ withEachNg1Version(() => { }); })); + it('should destroy the AngularJS app when `PlatformRef` is destroyed', waitForAsync(() => { + @Component({selector: 'ng2', template: 'NG2'}) + class Ng2Component { + } + + @NgModule({ + declarations: [Ng2Component], + entryComponents: [Ng2Component], + imports: [BrowserModule, UpgradeModule], + }) + class Ng2Module { + ngDoBootstrap() {} + } + + const ng1Module = angular.module_('ng1', []) + .component('ng1', {template: ''}) + .directive('ng2', downgradeComponent({component: Ng2Component})); + + const element = html('
'); + const platformRef = platformBrowserDynamic(); + + platformRef.bootstrapModule(Ng2Module).then(ref => { + const upgrade = ref.injector.get(UpgradeModule); + upgrade.bootstrap(element, [ng1Module.name]); + + const $rootScope: angular.IRootScopeService = upgrade.$injector.get($ROOT_SCOPE); + const rootScopeDestroySpy = spyOn($rootScope, '$destroy'); + + const appElem = angular.element(element); + const ng1Elem = angular.element(element.querySelector('ng1') as Element); + const ng2Elem = angular.element(element.querySelector('ng2') as Element); + const ng2ChildElem = angular.element(element.querySelector('ng2 span') as Element); + + // Attach data to all elements. + appElem.data!('testData', 1); + ng1Elem.data!('testData', 2); + ng2Elem.data!('testData', 3); + ng2ChildElem.data!('testData', 4); + + // Verify data can be retrieved. + expect(appElem.data!('testData')).toBe(1); + expect(ng1Elem.data!('testData')).toBe(2); + expect(ng2Elem.data!('testData')).toBe(3); + expect(ng2ChildElem.data!('testData')).toBe(4); + + expect(rootScopeDestroySpy).not.toHaveBeenCalled(); + + // Destroy `PlatformRef`. + platformRef.destroy(); + + // Verify `$rootScope` has been destroyed and data has been cleaned up. + expect(rootScopeDestroySpy).toHaveBeenCalled(); + + expect(appElem.data!('testData')).toBeUndefined(); + expect(ng1Elem.data!('testData')).toBeUndefined(); + expect(ng2Elem.data!('testData')).toBeUndefined(); + expect(ng2ChildElem.data!('testData')).toBeUndefined(); + }); + })); + it('should work when compiled outside the dom (by fallback to the root ng2.injector)', waitForAsync(() => { @Component({selector: 'ng2', template: 'test'}) diff --git a/packages/upgrade/static/test/integration/downgrade_module_spec.ts b/packages/upgrade/static/test/integration/downgrade_module_spec.ts index cc34ec57b7f5f..1b2f4b9f4f0dd 100644 --- a/packages/upgrade/static/test/integration/downgrade_module_spec.ts +++ b/packages/upgrade/static/test/integration/downgrade_module_spec.ts @@ -1353,6 +1353,68 @@ withEachNg1Version(() => { setTimeout(() => expect($injectorFromNg2).toBe($injectorFromNg1)); })); + it('should destroy the AngularJS app when `PlatformRef` is destroyed', waitForAsync(() => { + @Component({selector: 'ng2', template: 'NG2'}) + class Ng2Component { + } + + @NgModule({ + declarations: [Ng2Component], + entryComponents: [Ng2Component], + imports: [BrowserModule], + }) + class Ng2Module { + ngDoBootstrap() {} + } + + const bootstrapFn = (extraProviders: StaticProvider[]) => + platformBrowserDynamic(extraProviders).bootstrapModule(Ng2Module); + const lazyModuleName = downgradeModule(bootstrapFn); + const ng1Module = + angular.module_('ng1', [lazyModuleName]) + .component('ng1', {template: ''}) + .directive( + 'ng2', downgradeComponent({component: Ng2Component, propagateDigest})); + + const element = html('
'); + const $injector = angular.bootstrap(element, [ng1Module.name]); + + setTimeout(() => { // Wait for the module to be bootstrapped. + const $rootScope: angular.IRootScopeService = $injector.get($ROOT_SCOPE); + const rootScopeDestroySpy = spyOn($rootScope, '$destroy'); + + const appElem = angular.element(element); + const ng1Elem = angular.element(element.querySelector('ng1') as Element); + const ng2Elem = angular.element(element.querySelector('ng2') as Element); + const ng2ChildElem = angular.element(element.querySelector('ng2 span') as Element); + + // Attach data to all elements. + appElem.data!('testData', 1); + ng1Elem.data!('testData', 2); + ng2Elem.data!('testData', 3); + ng2ChildElem.data!('testData', 4); + + // Verify data can be retrieved. + expect(appElem.data!('testData')).toBe(1); + expect(ng1Elem.data!('testData')).toBe(2); + expect(ng2Elem.data!('testData')).toBe(3); + expect(ng2ChildElem.data!('testData')).toBe(4); + + expect(rootScopeDestroySpy).not.toHaveBeenCalled(); + + // Destroy `PlatformRef`. + getPlatform()?.destroy(); + + // Verify `$rootScope` has been destroyed and data has been cleaned up. + expect(rootScopeDestroySpy).toHaveBeenCalled(); + + expect(appElem.data!('testData')).toBeUndefined(); + expect(ng1Elem.data!('testData')).toBeUndefined(); + expect(ng2Elem.data!('testData')).toBeUndefined(); + expect(ng2ChildElem.data!('testData')).toBeUndefined(); + }); + })); + describe('(common error)', () => { let Ng2CompA: Type; let Ng2CompB: Type; From 885710a3082e3469f09bdf2ab9bc90983750f06c Mon Sep 17 00:00:00 2001 From: George Kalpakas Date: Thu, 10 Dec 2020 19:13:33 +0200 Subject: [PATCH 5/5] fixup! fix(upgrade): fix HMR for hybrid applications --- packages/upgrade/src/common/src/util.ts | 12 +++++++++--- packages/upgrade/src/dynamic/src/upgrade_adapter.ts | 5 +++-- packages/upgrade/static/src/downgrade_module.ts | 5 +++-- packages/upgrade/static/src/upgrade_module.ts | 5 +++-- 4 files changed, 18 insertions(+), 9 deletions(-) diff --git a/packages/upgrade/src/common/src/util.ts b/packages/upgrade/src/common/src/util.ts index ebb81c311c54f..1ac7ecb573d85 100644 --- a/packages/upgrade/src/common/src/util.ts +++ b/packages/upgrade/src/common/src/util.ts @@ -48,9 +48,15 @@ export function controllerKey(name: string): string { return '$' + name + 'Controller'; } -// Destroy an AngularJS app given the app `$injector`. -// -// NOTE: Destroying an app is not officially supported by AngularJS, but we do our best. +/** + * Destroy an AngularJS app given the app `$injector`. + * + * NOTE: Destroying an app is not officially supported by AngularJS, but try to do our best by + * destroying `$rootScope` and clean the jqLite/jQuery data on `$rootElement` and all + * descendants. + * + * @param $injector The `$injector` of the AngularJS app to destroy. + */ export function destroyApp($injector: IInjectorService): void { const $rootElement: IAugmentedJQuery = $injector.get($ROOT_ELEMENT); const $rootScope: IRootScopeService = $injector.get($ROOT_SCOPE); diff --git a/packages/upgrade/src/dynamic/src/upgrade_adapter.ts b/packages/upgrade/src/dynamic/src/upgrade_adapter.ts index bf5edf0f2d685..beb411505fe35 100644 --- a/packages/upgrade/src/dynamic/src/upgrade_adapter.ts +++ b/packages/upgrade/src/dynamic/src/upgrade_adapter.ts @@ -622,8 +622,9 @@ export class UpgradeAdapter { // Destroy the AngularJS app once the Angular `PlatformRef` is destroyed. // This does not happen in a typical SPA scenario, but it might be useful for - // other usecases where desposing of an Angular/AngularJS app is necessary (such - // as Hot Module Replacement (HMR)). + // other use-cases where disposing of an Angular/AngularJS app is necessary + // (such as Hot Module Replacement (HMR)). + // See https://github.com/angular/angular/issues/39935. platformRef.onDestroy(() => destroyApp(ng1Injector)); }); }) diff --git a/packages/upgrade/static/src/downgrade_module.ts b/packages/upgrade/static/src/downgrade_module.ts index 070fd23d7e4fd..cfec3e94d0208 100644 --- a/packages/upgrade/static/src/downgrade_module.ts +++ b/packages/upgrade/static/src/downgrade_module.ts @@ -169,8 +169,9 @@ export function downgradeModule(moduleFactoryOrBootstrapFn: NgModuleFactory destroyApp($injector)); return injector; diff --git a/packages/upgrade/static/src/upgrade_module.ts b/packages/upgrade/static/src/upgrade_module.ts index 2ae8df10a50a5..1e54a7834a8c1 100644 --- a/packages/upgrade/static/src/upgrade_module.ts +++ b/packages/upgrade/static/src/upgrade_module.ts @@ -259,8 +259,9 @@ export class UpgradeModule { // Destroy the AngularJS app once the Angular `PlatformRef` is destroyed. // This does not happen in a typical SPA scenario, but it might be useful for - // other usecases where desposing of an Angular/AngularJS app is necessary (such - // as Hot Module Replacement (HMR)). + // other use-cases where disposing of an Angular/AngularJS app is necessary + // (such as Hot Module Replacement (HMR)). + // See https://github.com/angular/angular/issues/39935. this.platformRef.onDestroy(() => destroyApp($injector)); // Wire up the ng1 rootScope to run a digest cycle whenever the zone settles