René's URL Explorer Experiment

Title: fix: fix memory leak on SSR by ValentinFunk · Pull Request #2243 · angular/angularfire · GitHub

Open Graph Title: fix: fix memory leak on SSR by ValentinFunk · Pull Request #2243 · angular/angularfire

X Title: fix: fix memory leak on SSR by ValentinFunk · Pull Request #2243 · angular/angularfire

Description: Checklist Issue number for this PR: TBD Docs included?: no - API did not change Test units included?: yes In a clean directory, yarn install, yarn test run successfully? Yes + manual testing in larger app (mainly auth/database) Description This is bug fix to fix an issue where running SSR with Angularfire would cause a memory leak, preventing the Application Module from being collected. The leak was caused by a) subscriptions leaking in the implementation of keepUnstableUntilFirst (https://github.com/angular/angularfire/blob/master/src/core/angularfire2.ts#L19, https://github.com/angular/angularfire/blob/master/src/core/angularfire2.ts#L54) b) References to the subscriber kept around in the noop function Explanation of the fix and approach When investigating this I found that to solve the leak a lot of "null" setting and more hacks on the code were required. I decided to try and improve the hack (https://github.com/angular/angularfire/blob/master/src/core/angularfire2.ts#L13) by makeing it a bit more idiomatic. To make universal work correctly the following is needed: Make sure that firebase library calls are done outside the Angular Zone. Otherwise the zone doesn't stabilize due to keepalive timers and websockets that are firebase opens. When in SSR, make sure that the angular Zone is blocked by a macrotask until the observable emits an item, completes or errors. This makes sure that SSR pages don't finish rendering prematurely, before data has arrived from firebase. 1) Firebase library calls done outside the Angular Zone The code was slightly refactored to allow passing an optional Scheduler as parameter to the library factory methods. This allows to easily specify the scheduler used for emissions of items from e.g. fromRef. This is in line with the RxJS api (e,g. fromPromise(promise, scheduler?). By using subscribeOn combined with a Scheduler that schedules work outside of the Angular Zone, the behavior can easily be achieved. An implementation of such a scheduler has been added as "ZoneScheduler". Related Tests: https://github.com/Kamshak/angularfire/blob/5a9550c993bc4ecf512c3ed5a4a2c25390d47009/src/database/observable/fromRef.spec.ts#L59 https://github.com/Kamshak/angularfire/blob/5a9550c993bc4ecf512c3ed5a4a2c25390d47009/src/database/observable/fromRef.spec.ts#L44 2) Preventing Zone from stabilizing until first item is emitted In order to solve the memory leak and make it difficult to accidentally introduce a new one, the approach is to use pipeable operators where possible, this reduces the complexity needed to manage subscriptions split functionality into small, independent units (Scheduler, operator, operator factory) The gist is this: return function keepUnstableUntilFirst(obs$: Observable): Observable { const inCorrectZones$ = obs$.pipe( // Run the observable creation outside the angular zone subscribeOn(schedulers.outsideAngular), // Run operators inside the angular zone (e.g. side effects via tap()) observeOn(schedulers.insideAngular) ); if (isPlatformServer(platformId)) { return inCorrectZones$.pipe( blockUntilFirst(schedulers.ngZone) ); } else { return inCorrectZones$; } } Full, commented implementation: https://github.com/angular/angularfire/blob/5a9550c993bc4ecf512c3ed5a4a2c25390d47009/src/core/angularfire2.ts These remaining changes from the second commit are only chore work/refactoring to make sure the Schedulers are available where needed. Testing Comprehensive testing was added to verify the functionality at each level (the intended behavior hasn't changed from the description in the PR). Most notably keepUnstableUntilFirstFactory now has a test that verfies that it creates a macrotask until the first emission, and removes it after. ZoneScheduler ✔ should schedule all tasks asynchronously ✔ should execute the scheduled work inside the specified zone ✔ should execute nested scheduled work inside the specified zone keepUnstableUntilFirstFactory ✔ should subscribe outside angular and observe inside angular (server) ✔ should subscribe outside angular and observe inside angular (browser) ✔ should block until first emission on server platform ✔ should not block on client platform fromRef ✔ should take a scheduler ✔ should schedule completed and error correctly Updated universal-test app In the third commit the universal-test app was updated to Angular 6 to make it work with @angular/fire again. The package.json command has been updated as well, however I did not add it to the travis file (not sure if you would like it tested on CI). No functional changes, testing works again and renders a html file. NPM version bumped to 5.2.4* Just a heads up - this was mainly for manual testing with yarn pack - suggesting a patch or minor bump. Also done in 2nd commit.

Open Graph Description: Checklist Issue number for this PR: TBD Docs included?: no - API did not change Test units included?: yes In a clean directory, yarn install, yarn test run successfully? Yes + manual testing in la...

X Description: Checklist Issue number for this PR: TBD Docs included?: no - API did not change Test units included?: yes In a clean directory, yarn install, yarn test run successfully? Yes + manual testing in la...

Opengraph URL: https://github.com/angular/angularfire/pull/2243

X: @github

direct link

Domain: github.com

Links:

Viewport: width=device-width