Skip to content

Commit 835b4d2

Browse files
remackgeekremackgeek
committed
fix(elements): run strategy methods in correct zone
Default change detection fails in some cases for @angular/elements where component events are called from the wrong zone. This fixes the issue by running all ComponentNgElementStrategy methods in the same zone it was created in. Fixes angular#24181
1 parent eee2fd2 commit 835b4d2

File tree

2 files changed

+123
-44
lines changed

2 files changed

+123
-44
lines changed

‎packages/elements/src/component-factory-strategy.ts

+71-41
Original file line numberDiff line numberDiff line change
@@ -6,7 +6,7 @@
66
* found in the LICENSE file at https://angular.io/license
77
*/
88

9-
import {ApplicationRef, ComponentFactory, ComponentFactoryResolver, ComponentRef, EventEmitter, Injector, OnChanges, SimpleChange, SimpleChanges, Type} from '@angular/core';
9+
import {ApplicationRef, ComponentFactory, ComponentFactoryResolver, ComponentRef, EventEmitter, Injector, NgZone, OnChanges, SimpleChange, SimpleChanges, Type} from '@angular/core';
1010
import {merge, Observable} from 'rxjs';
1111
import {map} from 'rxjs/operators';
1212

@@ -71,78 +71,108 @@ export class ComponentNgElementStrategy implements NgElementStrategy {
7171
*/
7272
private readonly unchangedInputs = new Set<string>();
7373

74-
constructor(private componentFactory: ComponentFactory<any>, private injector: Injector) {}
74+
/** check for zone.js loaded */
75+
private readonly zoneless: boolean;
76+
77+
/** service for setting zone context */
78+
private readonly ngZone: NgZone;
79+
80+
/** the zone the element was created in */
81+
private readonly elementZone: Zone|undefined;
82+
83+
constructor(private componentFactory: ComponentFactory<any>, private injector: Injector) {
84+
this.zoneless = typeof Zone == 'undefined';
85+
this.ngZone = this.injector.get<NgZone>(NgZone);
86+
if (!this.zoneless) {
87+
this.elementZone = this.ngZone.run(() => {
88+
return Zone.current;
89+
});
90+
}
91+
}
7592

7693
/**
7794
* Initializes a new component if one has not yet been created and cancels any scheduled
7895
* destruction.
7996
*/
8097
connect(element: HTMLElement) {
81-
// If the element is marked to be destroyed, cancel the task since the component was reconnected
82-
if (this.scheduledDestroyFn !== null) {
83-
this.scheduledDestroyFn();
84-
this.scheduledDestroyFn = null;
85-
return;
86-
}
98+
this.runInZone(() => {
99+
// If the element is marked to be destroyed, cancel the task since the component was
100+
// reconnected
101+
if (this.scheduledDestroyFn !== null) {
102+
this.scheduledDestroyFn();
103+
this.scheduledDestroyFn = null;
104+
return;
105+
}
87106

88-
if (this.componentRef === null) {
89-
this.initializeComponent(element);
90-
}
107+
if (this.componentRef === null) {
108+
this.initializeComponent(element);
109+
}
110+
});
91111
}
92112

93113
/**
94114
* Schedules the component to be destroyed after some small delay in case the element is just
95115
* being moved across the DOM.
96116
*/
97117
disconnect() {
98-
// Return if there is no componentRef or the component is already scheduled for destruction
99-
if (this.componentRef === null || this.scheduledDestroyFn !== null) {
100-
return;
101-
}
102-
103-
// Schedule the component to be destroyed after a small timeout in case it is being
104-
// moved elsewhere in the DOM
105-
this.scheduledDestroyFn = scheduler.schedule(() => {
106-
if (this.componentRef !== null) {
107-
this.componentRef.destroy();
108-
this.componentRef = null;
118+
this.runInZone(() => {
119+
// Return if there is no componentRef or the component is already scheduled for destruction
120+
if (this.componentRef === null || this.scheduledDestroyFn !== null) {
121+
return;
109122
}
110-
}, DESTROY_DELAY);
123+
124+
// Schedule the component to be destroyed after a small timeout in case it is being
125+
// moved elsewhere in the DOM
126+
this.scheduledDestroyFn = scheduler.schedule(() => {
127+
if (this.componentRef !== null) {
128+
this.componentRef.destroy();
129+
this.componentRef = null;
130+
}
131+
}, DESTROY_DELAY);
132+
});
111133
}
112134

113135
/**
114136
* Returns the component property value. If the component has not yet been created, the value is
115137
* retrieved from the cached initialization values.
116138
*/
117139
getInputValue(property: string): any {
118-
if (this.componentRef === null) {
119-
return this.initialInputValues.get(property);
120-
}
140+
return this.runInZone(() => {
141+
if (this.componentRef === null) {
142+
return this.initialInputValues.get(property);
143+
}
121144

122-
return this.componentRef.instance[property];
145+
return this.componentRef.instance[property];
146+
});
123147
}
124148

125149
/**
126150
* Sets the input value for the property. If the component has not yet been created, the value is
127151
* cached and set when the component is created.
128152
*/
129153
setInputValue(property: string, value: any): void {
130-
if (this.componentRef === null) {
131-
this.initialInputValues.set(property, value);
132-
return;
133-
}
154+
this.runInZone(() => {
155+
if (this.componentRef === null) {
156+
this.initialInputValues.set(property, value);
157+
return;
158+
}
134159

135-
// Ignore the value if it is strictly equal to the current value, except if it is `undefined`
136-
// and this is the first change to the value (because an explicit `undefined` _is_ strictly
137-
// equal to not having a value set at all, but we still need to record this as a change).
138-
if (strictEquals(value, this.getInputValue(property)) &&
139-
!((value === undefined) && this.unchangedInputs.has(property))) {
140-
return;
141-
}
160+
// Ignore the value if it is strictly equal to the current value, except if it is `undefined`
161+
// and this is the first change to the value (because an explicit `undefined` _is_ strictly
162+
// equal to not having a value set at all, but we still need to record this as a change).
163+
if (strictEquals(value, this.getInputValue(property)) &&
164+
!((value === undefined) && this.unchangedInputs.has(property))) {
165+
return;
166+
}
167+
168+
this.recordInputChange(property, value);
169+
this.componentRef.instance[property] = value;
170+
this.scheduleDetectChanges();
171+
});
172+
}
142173

143-
this.recordInputChange(property, value);
144-
this.componentRef.instance[property] = value;
145-
this.scheduleDetectChanges();
174+
private runInZone(fn: () => any) {
175+
return (this.zoneless || Zone.current === this.elementZone) ? fn() : this.ngZone.run(fn);
146176
}
147177

148178
/**

‎packages/elements/test/component-factory-strategy_spec.ts

+52-3
Original file line numberDiff line numberDiff line change
@@ -20,22 +20,38 @@ describe('ComponentFactoryNgElementStrategy', () => {
2020
let injector: any;
2121
let componentRef: any;
2222
let applicationRef: any;
23+
let ngZone: any;
24+
let factoryResolver: any;
2325

2426
beforeEach(() => {
2527
factory = new FakeComponentFactory();
2628
componentRef = factory.componentRef;
2729

2830
applicationRef = jasmine.createSpyObj('applicationRef', ['attachView']);
31+
32+
ngZone = jasmine.createSpyObj('ngZone', ['run']);
33+
ngZone.run.and.callFake((fn: any) => {
34+
return fn();
35+
});
36+
2937
injector = jasmine.createSpyObj('injector', ['get']);
30-
injector.get.and.returnValue(applicationRef);
38+
injector.get.and.callFake(function(identify: any) {
39+
const name = identify && identify.name;
40+
if (name === 'ApplicationRef') {
41+
return applicationRef;
42+
} else if (name === 'NgZone') {
43+
return ngZone;
44+
} else if (name === 'ComponentFactoryResolver') {
45+
return factoryResolver;
46+
}
47+
});
3148

3249
strategy = new ComponentNgElementStrategy(factory, injector);
3350
});
3451

3552
it('should create a new strategy from the factory', () => {
36-
const factoryResolver = jasmine.createSpyObj('factoryResolver', ['resolveComponentFactory']);
53+
factoryResolver = jasmine.createSpyObj('factoryResolver', ['resolveComponentFactory']);
3754
factoryResolver.resolveComponentFactory.and.returnValue(factory);
38-
injector.get.and.returnValue(factoryResolver);
3955

4056
const strategyFactory = new ComponentNgElementStrategyFactory(FakeComponent, injector);
4157
expect(strategyFactory.create(injector)).toBeTruthy();
@@ -239,6 +255,39 @@ describe('ComponentFactoryNgElementStrategy', () => {
239255
expect(componentRef.destroy).toHaveBeenCalledTimes(1);
240256
}));
241257
});
258+
259+
describe('runInZone', () => {
260+
it('should have zones (baseline)', () => {
261+
expect(strategy['zoneless']).toEqual(false);
262+
expect(strategy['ngZone']).toBeTruthy();
263+
expect(strategy['elementZone']).toBeTruthy();
264+
});
265+
266+
it('should run already in original zone', () => {
267+
let called = false;
268+
269+
ngZone.run.calls.reset();
270+
271+
strategy['runInZone'](() => {
272+
called = true;
273+
});
274+
275+
expect(called).toEqual(true);
276+
expect(ngZone.run).not.toHaveBeenCalled();
277+
});
278+
279+
it('should run placed in original zone', () => {
280+
let called = false;
281+
ngZone.run.calls.reset();
282+
283+
Zone.root.run(() => (strategy['runInZone'](() => {
284+
called = true;
285+
})));
286+
287+
expect(called).toEqual(true);
288+
expect(ngZone.run).toHaveBeenCalled();
289+
});
290+
});
242291
});
243292

244293
export class FakeComponentWithoutNgOnChanges {

0 commit comments

Comments
 (0)