Skip to content

Commit 25e5b04

Browse files
fix(): SSR memory leaks (#2294)
Co-authored-by: Valentin Funk <[email protected]>
1 parent 0d95523 commit 25e5b04

39 files changed

+4732
-2009
lines changed

karma.conf.js

+1
Original file line numberDiff line numberDiff line change
@@ -19,6 +19,7 @@ module.exports = function(config) {
1919
'node_modules/zone.js/dist/jasmine-patch.js',
2020
'node_modules/zone.js/dist/async-test.js',
2121
'node_modules/zone.js/dist/fake-async-test.js',
22+
'node_modules/zone.js/dist/task-tracking.js',
2223

2324
'node_modules/rxjs/bundles/rxjs.umd.{js,map}',
2425

package.json

+1-1
Original file line numberDiff line numberDiff line change
@@ -9,7 +9,7 @@
99
"test:watch": "concurrently \"npm run build:watch\" \"npm run delayed_karma\"",
1010
"test:debug": "npm run build && karma start",
1111
"karma": "karma start",
12-
"test:universal": "npm run build && cp -R dist/packages-dist test/universal-test/node_modules/angularfire2 && cd test/universal-test && npm run prerender",
12+
"test:universal": "npm run build && cd test/universal-test && yarn && npm run prerender",
1313
"delayed_karma": "sleep 10 && karma start",
1414
"build": "rimraf dist && node tools/build.js && npm pack ./dist/packages-dist",
1515
"changelog": "conventional-changelog -p angular -i CHANGELOG.md -s -r 1",

src/analytics/analytics.service.ts

+6-4
Original file line numberDiff line numberDiff line change
@@ -1,8 +1,8 @@
11
import { Injectable, Optional, NgZone, OnDestroy, ComponentFactoryResolver, Inject, PLATFORM_ID, Injector, NgModuleFactory } from '@angular/core';
22
import { Subscription, from, Observable, of } from 'rxjs';
3-
import { filter, withLatestFrom, switchMap, map, tap, pairwise, startWith, groupBy, mergeMap } from 'rxjs/operators';
3+
import { filter, withLatestFrom, switchMap, map, tap, pairwise, startWith, groupBy, mergeMap, observeOn } from 'rxjs/operators';
44
import { Router, NavigationEnd, ActivationEnd, ROUTES } from '@angular/router';
5-
import { runOutsideAngular } from '@angular/fire';
5+
import { ɵAngularFireSchedulers } from '@angular/fire';
66
import { AngularFireAnalytics, DEBUG_MODE } from './analytics';
77
import { User } from 'firebase/app';
88
import { Title } from '@angular/platform-browser';
@@ -160,15 +160,17 @@ export class UserTrackingService implements OnDestroy {
160160
zone: NgZone,
161161
@Inject(PLATFORM_ID) platformId:Object
162162
) {
163+
const schedulers = new ɵAngularFireSchedulers(zone);
164+
163165
if (!isPlatformServer(platformId)) {
164166
zone.runOutsideAngular(() => {
165167
// @ts-ignore zap the import in the UMD
166168
this.disposable = from(import('firebase/auth')).pipe(
169+
observeOn(schedulers.outsideAngular),
167170
switchMap(() => analytics.app),
168171
map(app => app.auth()),
169172
switchMap(auth => new Observable<User|null>(auth.onAuthStateChanged.bind(auth))),
170-
switchMap(user => analytics.setUserId(user ? user.uid : null!)),
171-
runOutsideAngular(zone)
173+
switchMap(user => analytics.setUserId(user ? user.uid : null!))
172174
).subscribe();
173175
});
174176
}

src/analytics/analytics.ts

+6-4
Original file line numberDiff line numberDiff line change
@@ -1,8 +1,8 @@
11
import { Injectable, Inject, Optional, NgZone, InjectionToken, PLATFORM_ID } from '@angular/core';
22
import { of } from 'rxjs';
33
import { isPlatformBrowser } from '@angular/common';
4-
import { map, tap, shareReplay, switchMap } from 'rxjs/operators';
5-
import { FirebaseAppConfig, FirebaseOptions, runOutsideAngular, ɵlazySDKProxy, FirebaseAnalytics, FIREBASE_OPTIONS, FIREBASE_APP_NAME, _firebaseAppFactory } from '@angular/fire';
4+
import { map, tap, shareReplay, switchMap, observeOn } from 'rxjs/operators';
5+
import { FirebaseAppConfig, FirebaseOptions, ɵlazySDKProxy, FirebaseAnalytics, FIREBASE_OPTIONS, FIREBASE_APP_NAME, _firebaseAppFactory, ɵAngularFireSchedulers } from '@angular/fire';
66
import { analytics, app } from 'firebase';
77

88
export interface Config {[key:string]: any};
@@ -58,6 +58,8 @@ export class AngularFireAnalytics {
5858
zone: NgZone
5959
) {
6060

61+
const schedulers = new ɵAngularFireSchedulers(zone);
62+
6163
if (isPlatformBrowser(platformId)) {
6264

6365
window[DATA_LAYER_NAME] = window[DATA_LAYER_NAME] || [];
@@ -84,15 +86,15 @@ export class AngularFireAnalytics {
8486
if (debugModeEnabled) { this.updateConfig({ [DEBUG_MODE_KEY]: 1 }) }
8587

8688
const analytics = of(undefined).pipe(
89+
observeOn(schedulers.outsideAngular),
8790
// @ts-ignore zapping in the UMD in the build script
88-
switchMap(() => zone.runOutsideAngular(() => import('firebase/analytics'))),
91+
switchMap(() => import('firebase/analytics')),
8992
map(() => _firebaseAppFactory(options, zone, nameOrConfig)),
9093
// SEMVER no need to cast once we drop older Firebase
9194
map(app => <analytics.Analytics>app.analytics()),
9295
tap(analytics => {
9396
if (analyticsCollectionEnabled === false) { analytics.setAnalyticsCollectionEnabled(false) }
9497
}),
95-
runOutsideAngular(zone),
9698
shareReplay({ bufferSize: 1, refCount: false }),
9799
);
98100

src/auth/auth.ts

+10-19
Original file line numberDiff line numberDiff line change
@@ -1,7 +1,7 @@
11
import { Injectable, Inject, Optional, NgZone, PLATFORM_ID } from '@angular/core';
22
import { Observable, of, from } from 'rxjs';
33
import { switchMap } from 'rxjs/operators';
4-
import { FIREBASE_OPTIONS, FIREBASE_APP_NAME, FirebaseOptions, FirebaseAppConfig, FirebaseAuth, _firebaseAppFactory, FirebaseZoneScheduler } from '@angular/fire';
4+
import { FIREBASE_OPTIONS, FIREBASE_APP_NAME, FirebaseOptions, FirebaseAppConfig, FirebaseAuth, _firebaseAppFactory, ɵAngularFireSchedulers, ɵkeepUnstableUntilFirstFactory } from '@angular/fire';
55
import { User, auth } from 'firebase/app';
66

77
@Injectable()
@@ -38,31 +38,22 @@ export class AngularFireAuth {
3838
@Inject(FIREBASE_OPTIONS) options:FirebaseOptions,
3939
@Optional() @Inject(FIREBASE_APP_NAME) nameOrConfig:string|FirebaseAppConfig|null|undefined,
4040
@Inject(PLATFORM_ID) platformId: Object,
41-
private zone: NgZone
41+
zone: NgZone
4242
) {
43-
const scheduler = new FirebaseZoneScheduler(zone, platformId);
43+
const keepUnstableUntilFirst = ɵkeepUnstableUntilFirstFactory(new ɵAngularFireSchedulers(zone), platformId);
44+
4445
this.auth = zone.runOutsideAngular(() => {
4546
const app = _firebaseAppFactory(options, zone, nameOrConfig);
4647
return app.auth();
4748
});
4849

49-
this.authState = scheduler.keepUnstableUntilFirst(
50-
scheduler.runOutsideAngular(
51-
new Observable(subscriber => {
52-
const unsubscribe = this.auth.onAuthStateChanged(subscriber);
53-
return { unsubscribe };
54-
})
55-
)
56-
);
50+
this.authState = new Observable<User | null>(subscriber => {
51+
return zone.runOutsideAngular(() => this.auth.onIdTokenChanged(subscriber));
52+
}).pipe(keepUnstableUntilFirst);;
5753

58-
this.user = scheduler.keepUnstableUntilFirst(
59-
scheduler.runOutsideAngular(
60-
new Observable(subscriber => {
61-
const unsubscribe = this.auth.onIdTokenChanged(subscriber);
62-
return { unsubscribe };
63-
})
64-
)
65-
);
54+
this.user = new Observable<User | null>(subscriber => {
55+
return zone.runOutsideAngular(() => this.auth.onIdTokenChanged(subscriber));
56+
}).pipe(keepUnstableUntilFirst);
6657

6758
this.idToken = this.user.pipe(switchMap(user => {
6859
return user ? from(user.getIdToken()) : of(null)

src/core/angularfire2.spec.ts

+196-2
Original file line numberDiff line numberDiff line change
@@ -1,10 +1,14 @@
11
import { TestBed, inject } from '@angular/core/testing';
2-
import { PlatformRef, NgModule, CompilerFactory } from '@angular/core';
2+
import { PlatformRef, NgModule, CompilerFactory, NgZone } from '@angular/core';
33
import { FirebaseApp, AngularFireModule } from '@angular/fire';
4-
import { Subscription } from 'rxjs';
4+
import { Subscription, Observable, Subject, of } from 'rxjs';
55
import { COMMON_CONFIG } from './test-config';
66
import { BrowserModule } from '@angular/platform-browser';
77
import { database } from 'firebase/app';
8+
import { ɵZoneScheduler, ɵkeepUnstableUntilFirstFactory, ɵAngularFireSchedulers } from './angularfire2';
9+
import { ɵPLATFORM_BROWSER_ID, ɵPLATFORM_SERVER_ID } from '@angular/common';
10+
import { tap } from 'rxjs/operators';
11+
import { TestScheduler } from 'rxjs/testing';
812

913
describe('angularfire', () => {
1014
let subscription:Subscription;
@@ -40,6 +44,196 @@ describe('angularfire', () => {
4044
app.delete().then(done, done.fail);
4145
});
4246

47+
describe('ZoneScheduler', () => {
48+
it('should execute the scheduled work inside the specified zone', done => {
49+
let ngZone = Zone.current.fork({
50+
name: 'ngZone'
51+
});
52+
const rootZone = Zone.current;
53+
54+
// Mimic real behavior: Executing in Angular
55+
ngZone.run(() => {
56+
const outsideAngularScheduler = new ɵZoneScheduler(rootZone);
57+
outsideAngularScheduler.schedule(() => {
58+
expect(Zone.current.name).not.toEqual('ngZone');
59+
done();
60+
});
61+
});
62+
});
63+
64+
it('should execute nested scheduled work inside the specified zone', done => {
65+
const testScheduler = new TestScheduler(null!);
66+
testScheduler.run(helpers => {
67+
const outsideAngularScheduler = new ɵZoneScheduler(Zone.current, testScheduler);
68+
69+
let ngZone = Zone.current.fork({
70+
name: 'ngZone'
71+
});
72+
73+
let callbacksRan = 0;
74+
75+
// Mimic real behavior: Executing in Angular
76+
ngZone.run(() => {
77+
outsideAngularScheduler.schedule(() => {
78+
callbacksRan++;
79+
expect(Zone.current.name).not.toEqual('ngZone');
80+
81+
ngZone.run(() => {
82+
// Sync queueing
83+
outsideAngularScheduler.schedule(() => {
84+
callbacksRan++;
85+
expect(Zone.current.name).not.toEqual('ngZone');
86+
});
87+
88+
// Async (10ms delay) nested scheduling
89+
outsideAngularScheduler.schedule(() => {
90+
callbacksRan++;
91+
expect(Zone.current.name).not.toEqual('ngZone');
92+
}, 10);
93+
94+
// Simulate flush from inside angular-
95+
helpers.flush();
96+
done();
97+
expect(callbacksRan).toEqual(3);
98+
})
99+
});
100+
helpers.flush();
101+
});
102+
});
103+
})
104+
})
105+
106+
describe('keepUnstableUntilFirstFactory', () => {
107+
let schedulers: ɵAngularFireSchedulers;
108+
let outsideZone: Zone;
109+
let insideZone: Zone;
110+
beforeAll(() => {
111+
outsideZone = Zone.current;
112+
insideZone = Zone.current.fork({
113+
name: 'ngZone'
114+
});
115+
const ngZone = {
116+
run: insideZone.run.bind(insideZone),
117+
runGuarded: insideZone.runGuarded.bind(insideZone),
118+
runOutsideAngular: outsideZone.runGuarded.bind(outsideZone),
119+
runTask: insideZone.run.bind(insideZone)
120+
} as NgZone;
121+
schedulers = new ɵAngularFireSchedulers(ngZone);
122+
})
123+
124+
it('should re-schedule emissions asynchronously', done => {
125+
const keepUnstableOp = ɵkeepUnstableUntilFirstFactory(schedulers, ɵPLATFORM_SERVER_ID);
126+
127+
let ran = false;
128+
of(null).pipe(
129+
keepUnstableOp,
130+
tap(() => ran = true)
131+
).subscribe(() => {
132+
expect(ran).toEqual(true);
133+
done();
134+
}, () => fail("Should not error"));
135+
136+
expect(ran).toEqual(false);
137+
});
138+
139+
[ɵPLATFORM_SERVER_ID, ɵPLATFORM_BROWSER_ID].map(platformId =>
140+
it(`should subscribe outside angular and observe inside angular (${platformId})`, done => {
141+
const keepUnstableOp = ɵkeepUnstableUntilFirstFactory(schedulers, platformId);
142+
143+
insideZone.run(() => {
144+
new Observable(s => {
145+
expect(Zone.current).toEqual(outsideZone);
146+
s.next("test");
147+
}).pipe(
148+
keepUnstableOp,
149+
tap(() => {
150+
expect(Zone.current).toEqual(insideZone);
151+
})
152+
).subscribe(() => {
153+
expect(Zone.current).toEqual(insideZone);
154+
done();
155+
}, err => {
156+
fail(err);
157+
});
158+
});
159+
})
160+
);
161+
162+
it('should block until first emission on server platform', done => {
163+
const testScheduler = new TestScheduler(null!);
164+
testScheduler.run(helpers => {
165+
const outsideZone = Zone.current;
166+
const taskTrack = new Zone['TaskTrackingZoneSpec']();
167+
const insideZone = Zone.current.fork(taskTrack);
168+
const trackingSchedulers: ɵAngularFireSchedulers = {
169+
ngZone: {
170+
run: insideZone.run.bind(insideZone),
171+
runGuarded: insideZone.runGuarded.bind(insideZone),
172+
runOutsideAngular: outsideZone.runGuarded.bind(outsideZone),
173+
runTask: insideZone.run.bind(insideZone)
174+
} as NgZone,
175+
outsideAngular: new ɵZoneScheduler(outsideZone, testScheduler),
176+
insideAngular: new ɵZoneScheduler(insideZone, testScheduler),
177+
};
178+
const keepUnstableOp = ɵkeepUnstableUntilFirstFactory(trackingSchedulers, ɵPLATFORM_SERVER_ID);
179+
180+
const s = new Subject();
181+
s.pipe(
182+
keepUnstableOp,
183+
).subscribe(() => { }, err => { fail(err); }, () => { });
184+
185+
// Flush to ensure all async scheduled functions are run
186+
helpers.flush();
187+
// Should now be blocked until first item arrives
188+
expect(taskTrack.macroTasks.length).toBe(1);
189+
expect(taskTrack.macroTasks[0].source).toBe('firebaseZoneBlock');
190+
191+
// Emit next item
192+
s.next(123);
193+
helpers.flush();
194+
195+
// Should not be blocked after first item
196+
expect(taskTrack.macroTasks.length).toBe(0);
197+
198+
done();
199+
});
200+
})
201+
202+
it('should not block on client platform', done => {
203+
const testScheduler = new TestScheduler(null!);
204+
testScheduler.run(helpers => {
205+
const outsideZone = Zone.current;
206+
const taskTrack = new Zone['TaskTrackingZoneSpec']();
207+
const insideZone = Zone.current.fork(taskTrack);
208+
const trackingSchedulers: ɵAngularFireSchedulers = {
209+
ngZone: {
210+
run: insideZone.run.bind(insideZone),
211+
runGuarded: insideZone.runGuarded.bind(insideZone),
212+
runOutsideAngular: outsideZone.runGuarded.bind(outsideZone),
213+
runTask: insideZone.run.bind(insideZone)
214+
} as NgZone,
215+
outsideAngular: new ɵZoneScheduler(outsideZone, testScheduler),
216+
insideAngular: new ɵZoneScheduler(insideZone, testScheduler),
217+
};
218+
const keepUnstableOp = ɵkeepUnstableUntilFirstFactory(trackingSchedulers, ɵPLATFORM_BROWSER_ID);
219+
220+
const s = new Subject();
221+
s.pipe(
222+
keepUnstableOp,
223+
).subscribe(() => { }, err => { fail(err); }, () => { });
224+
225+
// Flush to ensure all async scheduled functions are run
226+
helpers.flush();
227+
228+
// Zone should not be blocked
229+
expect(taskTrack.macroTasks.length).toBe(0);
230+
expect(taskTrack.microTasks.length).toBe(0);
231+
232+
done();
233+
});
234+
})
235+
})
236+
43237
describe('FirebaseApp', () => {
44238
it('should provide a FirebaseApp for the FirebaseApp binding', () => {
45239
expect(typeof app.delete).toBe('function');

0 commit comments

Comments
 (0)