Apply strictNullChecks to all packages except database (#1805)

* TS changes before splitting database

* Roll back database changes

* [AUTOMATED]: Prettier Code Styling

* Put back database nullcheck exception

* Finish fixing strictNullCheck errors

* Fix logger test.

* Fix tests

* Update primitive array types to type[]

* [AUTOMATED]: Prettier Code Styling

* Put test exclusion back in rxfire

* Address PR comments

* [AUTOMATED]: Prettier Code Styling

* Addressing PR comments

* Few more comments to address

* change assertThrows to assume it never returns null

* [AUTOMATED]: Prettier Code Styling

* Fix additional warnings after merge

* Fix storage test

* [AUTOMATED]: Prettier Code Styling

* Address comments in PR

Author: hsubox76

config/tsconfig.base.json

@@ -3,6 +3,7 @@
   "compilerOptions": {
     "declaration": true,
     "importHelpers": true,
+    "strictNullChecks": true,
     "lib": [
       "es2015",
       "dom"

integration/typescript/test/namespace.test.ts

@@ -17,7 +17,7 @@
 
 import * as firebase from 'firebase';
 import * as namespaceDefinition from '../../shared/namespaceDefinition.json';
-import * as validateNamespace from '../../shared/validator';
+import validateNamespace from '../../shared/validator';
 
 firebase.initializeApp({
   apiKey: 'test-api-key',

integration/typescript/tsconfig.json

@@ -9,7 +9,8 @@
     "noImplicitAny": true,
     "outDir": "dist",
     "target": "ES5",
-    "sourceMap": true
+    "sourceMap": true,
+    "esModuleInterop": true
   },
   "exclude": [
     "node_modules",

packages/app/src/firebaseApp.ts

@@ -114,9 +114,9 @@ export class FirebaseAppImpl implements FirebaseApp {
         }
 
         return Promise.all(
-          services.map(service => {
-            return service.INTERNAL.delete();
-          })
+          services
+            .filter(service => 'INTERNAL' in service)
+            .map(service => service.INTERNAL!.delete())
         );
       })
       .then(

packages/app/src/lite/firebaseAppLite.ts

@@ -96,9 +96,9 @@ export class FirebaseAppLiteImpl implements FirebaseApp {
         }
 
         return Promise.all(
-          services.map(service => {
-            return service.INTERNAL.delete();
-          })
+          services
+            .filter(service => 'INTERNAL' in service)
+            .map(service => service.INTERNAL!.delete())
         );
       })
       .then(

packages/app/test/firebaseApp.test.ts

@@ -161,8 +161,8 @@ function executeFirebaseTests() {
           const [app, , instanceIdentifier] = args;
           return new TestService(app, instanceIdentifier);
         },
-        null,
-        null,
+        undefined,
+        undefined,
         true
       );
       firebase.initializeApp({});
@@ -200,8 +200,8 @@ function executeFirebaseTests() {
           const [app, , instanceIdentifier] = args;
           return new TestService(app, instanceIdentifier);
         },
-        null,
-        null,
+        undefined,
+        undefined,
         false // <-- multi instance flag
       );
       firebase.initializeApp({});
@@ -268,11 +268,13 @@ function executeFirebaseTests() {
       // service's value.
       return (app as _FirebaseApp).INTERNAL.getToken()
         .then(token => {
-          assert.equal('tokenFor0', token.accessToken);
+          assert.isNotNull(token);
+          assert.equal('tokenFor0', token!.accessToken);
           return (app2 as _FirebaseApp).INTERNAL.getToken();
         })
         .then(token => {
-          assert.equal('tokenFor1', token.accessToken);
+          assert.isNotNull(token);
+          assert.equal('tokenFor1', token!.accessToken);
         });
     });
   });

packages/database/tsconfig.json

@@ -1,7 +1,8 @@
 {
   "extends": "../../config/tsconfig.base.json",
   "compilerOptions": {
-    "outDir": "dist"
+    "outDir": "dist",
+    "strictNullChecks": false
   },
   "exclude": [
     "dist/**/*"

packages/functions/test/callable.test.ts

@@ -69,7 +69,7 @@ describe('Firebase Functions > Call', () => {
     functions = new Service(app, region);
     if (useEmulator) {
       functions.useFunctionsEmulator(
-        process.env.FIREBASE_FUNCTIONS_EMULATOR_ORIGIN
+        process.env.FIREBASE_FUNCTIONS_EMULATOR_ORIGIN!
       );
     }
   });

packages/functions/test/service.test.ts

@@ -39,7 +39,6 @@ describe('Firebase Functions > Service', () => {
         service._url('foo'),
         'http://localhost:5005/my-project/us-central1/foo'
       );
-      service.useFunctionsEmulator(null);
     });
   });
 

packages/logger/test/logger.test.ts

@@ -16,31 +16,28 @@
  */
 
 import { expect } from 'chai';
-import { spy as Spy } from 'sinon';
+import { spy as Spy, SinonSpy } from 'sinon';
 import { Logger, LogLevel } from '../src/logger';
 import { setLogLevel } from '../index';
 import { debug } from 'util';
 
 describe('@firebase/logger', () => {
   const message = 'Hello there!';
   let client: Logger;
-  const spies = {
-    logSpy: null,
-    infoSpy: null,
-    warnSpy: null,
-    errorSpy: null
-  };
+  let spies: { [key: string]: SinonSpy };
   /**
    * Before each test, instantiate a new instance of Logger and establish spies
    * on all of the console methods so we can assert against them as needed
    */
   beforeEach(() => {
     client = new Logger('@firebase/test-logger');
 
-    spies.logSpy = Spy(console, 'log');
-    spies.infoSpy = Spy(console, 'info');
-    spies.warnSpy = Spy(console, 'warn');
-    spies.errorSpy = Spy(console, 'error');
+    spies = {
+      logSpy: Spy(console, 'log'),
+      infoSpy: Spy(console, 'info'),
+      warnSpy: Spy(console, 'warn'),
+      errorSpy: Spy(console, 'error')
+    };
   });
 
   afterEach(() => {
@@ -62,7 +59,7 @@ describe('@firebase/logger', () => {
     } call \`console.${channel}\` if \`.${channel}\` is called`, () => {
       client[channel](message);
       expect(
-        spies[`${channel}Spy`] && spies[`${channel}Spy`].called,
+        spies[`${channel}Spy`]!.called,
         `Expected ${channel} to ${shouldLog ? '' : 'not'} log`
       ).to.be[shouldLog ? 'true' : 'false'];
     });

packages/rxfire/database/list/index.ts

@@ -18,7 +18,7 @@
 import { database } from 'firebase';
 import { QueryChange, ListenEvent } from '../interfaces';
 import { Observable, of, merge, from } from 'rxjs';
-import { validateEventsArray, isNil } from '../utils';
+import { validateEventsArray } from '../utils';
 import { fromRef } from '../fromRef';
 import { switchMap, scan, distinctUntilChanged, map } from 'rxjs/operators';
 import { changeToData } from '../object';
@@ -42,11 +42,13 @@ export function list(
   query: database.Query,
   events?: ListenEvent[]
 ): Observable<QueryChange[]> {
-  events = validateEventsArray(events);
+  const eventsList = validateEventsArray(events);
   return fromOnce(query).pipe(
     switchMap(change => {
       const childEvent$ = [of(change)];
-      events.forEach(event => childEvent$.push(fromRef(query, event)));
+      for (const event of eventsList) {
+        childEvent$.push(fromRef(query, event));
+      }
       return merge(...childEvent$).pipe(scan(buildView, []));
     }),
     distinctUntilChanged()
@@ -78,7 +80,7 @@ function positionFor(changes: QueryChange[], key) {
 }
 
 function positionAfter(changes: QueryChange[], prevKey?: string) {
-  if (isNil(prevKey)) {
+  if (prevKey == null) {
     return 0;
   } else {
     const i = positionFor(changes, prevKey);
@@ -94,11 +96,11 @@ function buildView(current: QueryChange[], change: QueryChange) {
   const { snapshot, prevKey, event } = change;
   const { key } = snapshot;
   const currentKeyPosition = positionFor(current, key);
-  const afterPreviousKeyPosition = positionAfter(current, prevKey);
+  const afterPreviousKeyPosition = positionAfter(current, prevKey || undefined);
   switch (event) {
     case ListenEvent.value:
       if (change.snapshot && change.snapshot.exists()) {
-        let prevKey = null;
+        let prevKey: string | null = null;
         change.snapshot.forEach(snapshot => {
           const action: QueryChange = {
             snapshot,

packages/rxfire/database/utils.ts

@@ -17,17 +17,13 @@
 
 import { ListenEvent } from './interfaces';
 
-export function isNil(obj: any): boolean {
-  return obj === undefined || obj === null;
-}
-
 /**
  * Check the length of the provided array. If it is empty return an array
  * that is populated with all the Realtime Database child events.
  * @param events
  */
-export function validateEventsArray(events?: ListenEvent[]) {
-  if (isNil(events) || events!.length === 0) {
+export function validateEventsArray(events?: ListenEvent[]): ListenEvent[] {
+  if (events == null || events.length === 0) {
     events = [
       ListenEvent.added,
       ListenEvent.removed,

packages/rxfire/firestore/collection/index.ts

@@ -37,7 +37,7 @@ const filterEvents = (events?: firestore.DocumentChangeType[]) =>
     let hasChange = false;
     for (let i = 0; i < changes.length; i++) {
       const change = changes[i];
-      if (events.indexOf(change.type) >= 0) {
+      if (events && events.indexOf(change.type) >= 0) {
         hasChange = true;
         break;
       }

packages/rxfire/firestore/document/index.ts

@@ -36,7 +36,7 @@ export function docData<T>(
 }
 
 export function snapToData(
-  snapshot: firestore.QueryDocumentSnapshot,
+  snapshot: firestore.DocumentSnapshot,
   idField?: string
 ) {
   return {

packages/rxfire/test/database.test.ts

@@ -41,11 +41,11 @@ let batch = (items: any[]) => {
 };
 
 describe('RxFire Database', () => {
-  let app: app.App = null;
-  let database: database.Database = null;
+  let app: app.App;
+  let database: database.Database;
   let ref = (path: string) => {
-    app.database().goOffline();
-    return app.database().ref(path);
+    app!.database().goOffline();
+    return app!.database().ref(path);
   };
 
   function prepareList(
@@ -147,7 +147,7 @@ describe('RxFire Database', () => {
           count = count + 1;
           const { event, snapshot } = change;
           expect(event).to.equal(ListenEvent.added);
-          expect(snapshot.val()).to.eql(data[snapshot.key]);
+          expect(snapshot.val()).to.eql(data[snapshot.key!]);
           if (count === items.length) {
             done();
             sub.unsubscribe();

packages/rxfire/test/firestore.test.ts

@@ -67,8 +67,8 @@ const seedTest = firestore => {
 };
 
 describe('RxFire Firestore', () => {
-  let app: app.App = null;
-  let firestore: firestore.Firestore = null;
+  let app: app.App;
+  let firestore: firestore.Firestore;
 
   /**
    * Each test runs inside it's own app instance and the app

packages/storage/src/implementation/list.ts

@@ -23,6 +23,7 @@ import { Location } from './location';
 import * as json from './json';
 import * as type from './type';
 import { ListResult } from '../list';
+import * as errors from './error';
 
 /**
  * Represents the simplified object metadata returned by List API.
@@ -58,11 +59,15 @@ function fromBackendResponse(
     items: [],
     nextPageToken: resource['nextPageToken']
   };
+  const bucket = authWrapper.bucket();
+  if (bucket === null) {
+    throw errors.noDefaultBucket();
+  }
   if (resource[PREFIXES_KEY]) {
     for (const path of resource[PREFIXES_KEY]) {
       const pathWithoutTrailingSlash = path.replace(/\/$/, '');
       const reference = authWrapper.makeStorageReference(
-        new Location(authWrapper.bucket(), pathWithoutTrailingSlash)
+        new Location(bucket, pathWithoutTrailingSlash)
       );
       listResult.prefixes.push(reference);
     }
@@ -71,7 +76,7 @@ function fromBackendResponse(
   if (resource[ITEMS_KEY]) {
     for (const item of resource[ITEMS_KEY]) {
       const reference = authWrapper.makeStorageReference(
-        new Location(authWrapper.bucket(), item['name'])
+        new Location(bucket, item['name'])
       );
       listResult.items.push(reference);
     }

packages/storage/src/implementation/location.ts

@@ -67,7 +67,7 @@ export class Location {
   }
 
   static makeFromUrl(url: string): Location {
-    let location = null;
+    let location: Location | null = null;
     let bucketDomain = '([A-Za-z0-9.\\-_]+)';
 
     function gsModify(loc: Location) {

packages/storage/src/implementation/metadata.ts

@@ -70,7 +70,7 @@ export function getMappings(): Mappings {
   if (mappings_) {
     return mappings_;
   }
-  let mappings = [];
+  const mappings: Mapping[] = [];
   mappings.push(new Mapping('bucket'));
   mappings.push(new Mapping('generation'));
   mappings.push(new Mapping('metageneration'));

packages/storage/src/implementation/observer.ts

@@ -15,16 +15,17 @@
  * limitations under the License.
  */
 import * as type from './type';
+import { FirebaseStorageError } from './error';
 
 type NextFn<T> = (value: T) => void;
-type ErrorFn = (error: Error) => void;
+type ErrorFn = (error: Error | FirebaseStorageError) => void;
 type CompleteFn = () => void;
 type Unsubscribe = () => void;
 
 type Subscribe<T> = (
-  next: NextFn<T> | { [name: string]: string | null },
-  error?: ErrorFn,
-  complete?: CompleteFn
+  next?: NextFn<T> | { [name: string]: string | null } | null,
+  error?: ErrorFn | null,
+  complete?: CompleteFn | null
 ) => Unsubscribe;
 
 export { NextFn, ErrorFn, CompleteFn, Unsubscribe, Subscribe };
@@ -33,12 +34,12 @@ export { NextFn, ErrorFn, CompleteFn, Unsubscribe, Subscribe };
  * @struct
  */
 export class Observer<T> {
-  next: NextFn<T> | null;
-  error: ErrorFn | null;
-  complete: CompleteFn | null;
+  next?: NextFn<T> | null;
+  error?: ErrorFn | null;
+  complete?: CompleteFn | null;
 
   constructor(
-    nextOrObserver: NextFn<T> | { [name: string]: string | null } | null,
+    nextOrObserver?: NextFn<T> | { [name: string]: string | null } | null,
     opt_error?: ErrorFn | null,
     opt_complete?: CompleteFn | null
   ) {