From b4b21bdff4b07fe9968c8d61fb9f5a2caa662331 Mon Sep 17 00:00:00 2001 From: George Kalpakas Date: Thu, 10 Dec 2020 19:13:33 +0200 Subject: [PATCH] fix(upgrade): fix HMR for hybrid applications (#40045) 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 PR Close #40045 --- goldens/public-api/upgrade/static/static.d.ts | 3 +- packages/upgrade/src/common/src/constants.ts | 1 + packages/upgrade/src/common/src/util.ts | 21 ++++++- .../src/dynamic/src/upgrade_adapter.ts | 9 ++- .../upgrade/src/dynamic/test/upgrade_spec.ts | 60 +++++++++++++++++- .../upgrade/static/src/downgrade_module.ts | 11 +++- packages/upgrade/static/src/upgrade_module.ts | 21 +++++-- .../integration/downgrade_component_spec.ts | 61 ++++++++++++++++++ .../test/integration/downgrade_module_spec.ts | 62 +++++++++++++++++++ 9 files changed, 238 insertions(+), 11 deletions(-) diff --git a/goldens/public-api/upgrade/static/static.d.ts b/goldens/public-api/upgrade/static/static.d.ts index 47b7425ce9..d21bc57848 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 7c9c5d0c8f..640d23adf0 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 bb93e23a63..1ac7ecb573 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,23 @@ 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 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); + + $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 0008620b12..beb411505f 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,13 @@ 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 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)); }); }) .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 3d33cc59be..70bbbc66e3 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 bde2fe8db9..cfec3e94d0 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,13 @@ 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 b3e32bc986..1e54a7834a 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,16 @@ 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 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 // 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 ec88d423d1..5076831f8f 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 cc34ec57b7..1b2f4b9f4f 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;