From f9bb53a761655e9671424e405bb4083a341845ad Mon Sep 17 00:00:00 2001 From: Andrew Kushnir Date: Thu, 18 Apr 2019 17:52:04 -0700 Subject: [PATCH] fix(ivy): allow TestBed.createComponent to create components in isolation (#29981) Prior to this change, components created via TestBed.createComponent in the same test were placed into the same root context, which caused problems in conjunction with fixture.autoDetectChanges usage in the same test. Specifically, change detection was triggered immediately for created component (starting from the 2nd one) even if it was not required/desired. This commit makes Ivy and VE behavior consistent: now every component created via TestBed.createComponent is isolated from each other. Current solution uses host element id naming convention, which is not ideal, but helps avoid public API surface changes at this point (we might revisit this approach later). Note: this commit also adds extra tests to verify bootstrap and change detection behavior in case of multiple components in `bootstrap` array in @NgModule, to make sure this behavior is aligned between Ivy and VE. PR Close #29981 --- packages/core/src/render3/component_ref.ts | 13 +++- packages/core/test/test_bed_spec.ts | 38 ++++++++- packages/core/testing/src/r3_test_bed.ts | 2 +- .../test/browser/bootstrap_spec.ts | 77 +++++++++++++++++++ 4 files changed, 126 insertions(+), 4 deletions(-) diff --git a/packages/core/src/render3/component_ref.ts b/packages/core/src/render3/component_ref.ts index 34d2d57e81..77fca80daf 100644 --- a/packages/core/src/render3/component_ref.ts +++ b/packages/core/src/render3/component_ref.ts @@ -149,8 +149,17 @@ export class ComponentFactory extends viewEngine_ComponentFactory { const rootFlags = this.componentDef.onPush ? LViewFlags.Dirty | LViewFlags.IsRoot : LViewFlags.CheckAlways | LViewFlags.IsRoot; - const rootContext: RootContext = - !isInternalRootView ? rootViewInjector.get(ROOT_CONTEXT) : createRootContext(); + + // Check whether this Component needs to be isolated from other components, i.e. whether it + // should be placed into its own (empty) root context or existing root context should be used. + // Note: this is internal-only convention and might change in the future, so it should not be + // relied upon externally. + const isIsolated = typeof rootSelectorOrNode === 'string' && + /^#root-ng-internal-isolated-\d+/.test(rootSelectorOrNode); + + const rootContext: RootContext = (isInternalRootView || isIsolated) ? + createRootContext() : + rootViewInjector.get(ROOT_CONTEXT); const renderer = rendererFactory.createRenderer(hostRNode, this.componentDef); diff --git a/packages/core/test/test_bed_spec.ts b/packages/core/test/test_bed_spec.ts index 2e14c67da8..fa546d6ac1 100644 --- a/packages/core/test/test_bed_spec.ts +++ b/packages/core/test/test_bed_spec.ts @@ -6,7 +6,7 @@ * found in the LICENSE file at https://angular.io/license */ -import {Component, Directive, ErrorHandler, Inject, Injectable, InjectionToken, NgModule, Optional, Pipe, ɵsetClassMetadata as setClassMetadata, ɵɵdefineComponent as defineComponent, ɵɵtext as text} from '@angular/core'; +import {Component, Directive, ErrorHandler, Inject, Injectable, InjectionToken, Input, NgModule, Optional, Pipe, ɵsetClassMetadata as setClassMetadata, ɵɵdefineComponent as defineComponent, ɵɵtext as text} from '@angular/core'; import {TestBed, getTestBed} from '@angular/core/testing/src/test_bed'; import {By} from '@angular/platform-browser'; import {expect} from '@angular/platform-browser/testing/src/matchers'; @@ -257,6 +257,42 @@ describe('TestBed', () => { expect(simpleApp.nativeElement).toHaveText('simple - inherited'); }); + it('should not trigger change detection for ComponentA while calling TestBed.createComponent for ComponentB', + () => { + const log: string[] = []; + @Component({ + selector: 'comp-a', + template: '...', + }) + class CompA { + @Input() inputA: string = ''; + ngOnInit() { log.push('CompA:ngOnInit', this.inputA); } + } + + @Component({ + selector: 'comp-b', + template: '...', + }) + class CompB { + @Input() inputB: string = ''; + ngOnInit() { log.push('CompB:ngOnInit', this.inputB); } + } + + TestBed.configureTestingModule({declarations: [CompA, CompB]}); + + log.length = 0; + const appA = TestBed.createComponent(CompA); + appA.componentInstance.inputA = 'a'; + appA.autoDetectChanges(); + expect(log).toEqual(['CompA:ngOnInit', 'a']); + + log.length = 0; + const appB = TestBed.createComponent(CompB); + appB.componentInstance.inputB = 'b'; + appB.autoDetectChanges(); + expect(log).toEqual(['CompB:ngOnInit', 'b']); + }); + it('should resolve components without async resources synchronously', (done) => { TestBed .configureTestingModule({ diff --git a/packages/core/testing/src/r3_test_bed.ts b/packages/core/testing/src/r3_test_bed.ts index 98d36d8bee..38f8e50abc 100644 --- a/packages/core/testing/src/r3_test_bed.ts +++ b/packages/core/testing/src/r3_test_bed.ts @@ -343,7 +343,7 @@ export class TestBedRender3 implements Injector, TestBed { createComponent(type: Type): ComponentFixture { const testComponentRenderer: TestComponentRenderer = this.get(TestComponentRenderer); - const rootElId = `root${_nextRootElementId++}`; + const rootElId = `root-ng-internal-isolated-${_nextRootElementId++}`; testComponentRenderer.insertRootElement(rootElId); const componentDef = (type as any).ngComponentDef; diff --git a/packages/platform-browser/test/browser/bootstrap_spec.ts b/packages/platform-browser/test/browser/bootstrap_spec.ts index d1f938742c..ce478f8782 100644 --- a/packages/platform-browser/test/browser/bootstrap_spec.ts +++ b/packages/platform-browser/test/browser/bootstrap_spec.ts @@ -470,5 +470,82 @@ function bootstrap( }); })); + describe('change detection', () => { + const log: string[] = []; + @Component({ + selector: 'hello-app', + template: '
{{title}}
', + }) + class CompA { + title: string = ''; + ngDoCheck() { log.push('CompA:ngDoCheck'); } + onClick() { + this.title = 'CompA'; + log.push('CompA:onClick'); + } + } + + @Component({ + selector: 'hello-app-2', + template: '
{{title}}
', + }) + class CompB { + title: string = ''; + ngDoCheck() { log.push('CompB:ngDoCheck'); } + onClick() { + this.title = 'CompB'; + log.push('CompB:onClick'); + } + } + + it('should be triggered for all bootstrapped components in case change happens in one of them', + inject([AsyncTestCompleter], (async: AsyncTestCompleter) => { + @NgModule({ + imports: [BrowserModule], + declarations: [CompA, CompB], + bootstrap: [CompA, CompB], + schemas: [CUSTOM_ELEMENTS_SCHEMA] + }) + class TestModuleA { + } + platformBrowserDynamic().bootstrapModule(TestModuleA).then((ref) => { + log.length = 0; + el.querySelectorAll('#button-a')[0].click(); + expect(log).toContain('CompA:onClick'); + expect(log).toContain('CompA:ngDoCheck'); + expect(log).toContain('CompB:ngDoCheck'); + + log.length = 0; + el2.querySelectorAll('#button-b')[0].click(); + expect(log).toContain('CompB:onClick'); + expect(log).toContain('CompA:ngDoCheck'); + expect(log).toContain('CompB:ngDoCheck'); + + async.done(); + }); + })); + + + it('should work in isolation for each component bootstrapped individually', + inject([AsyncTestCompleter], (async: AsyncTestCompleter) => { + const refPromise1 = bootstrap(CompA); + const refPromise2 = bootstrap(CompB); + Promise.all([refPromise1, refPromise2]).then((refs) => { + log.length = 0; + el.querySelectorAll('#button-a')[0].click(); + expect(log).toContain('CompA:onClick'); + expect(log).toContain('CompA:ngDoCheck'); + expect(log).not.toContain('CompB:ngDoCheck'); + + log.length = 0; + el2.querySelectorAll('#button-b')[0].click(); + expect(log).toContain('CompB:onClick'); + expect(log).toContain('CompB:ngDoCheck'); + expect(log).not.toContain('CompA:ngDoCheck'); + + async.done(); + }); + })); + }); }); }