Address PR comments

Author: hsubox76

packages/app/src/firebaseApp.ts

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

packages/logger/test/logger.test.ts

@@ -24,30 +24,27 @@ import { debug } from 'util';
 describe('@firebase/logger', () => {
   const message = 'Hello there!';
   let client: Logger;
-  const spies: { [key: string]: SinonSpy | null } = {
-    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(() => {
-    spies.logSpy && spies.logSpy.restore();
-    spies.infoSpy && spies.infoSpy.restore();
-    spies.warnSpy && spies.warnSpy.restore();
-    spies.errorSpy && spies.errorSpy.restore();
+    spies.logSpy.restore();
+    spies.infoSpy.restore();
+    spies.warnSpy.restore();
+    spies.errorSpy.restore();
   });
 
   function testLog(message, channel, shouldLog) {

packages/rxfire/database/list/index.ts

@@ -42,11 +42,13 @@ export function list(
   query: database.Query,
   events?: ListenEvent[]
 ): Observable<QueryChange[]> {
-  const eventsToList = validateEventsArray(events);
+  const eventsList = validateEventsArray(events);
   return fromOnce(query).pipe(
     switchMap(change => {
       const childEvent$ = [of(change)];
-      eventsToList.forEach(event => childEvent$.push(fromRef(query, event)));
+      for (const event of eventsList) {
+        childEvent$.push(fromRef(query, event));
+      }
       return merge(...childEvent$).pipe(scan(buildView, []));
     }),
     distinctUntilChanged()

packages/rxfire/database/utils.ts

@@ -17,7 +17,7 @@
 
 import { ListenEvent } from './interfaces';
 
-export function isNil(obj: any): boolean {
+export function isNil(obj: any): obj is null | undefined {
   return obj === undefined || obj === null;
 }
 
@@ -27,14 +27,13 @@ export function isNil(obj: any): boolean {
  * @param events
  */
 export function validateEventsArray(events?: ListenEvent[]): ListenEvent[] {
-  let validatedEvents: ListenEvent[] = [];
   if (isNil(events) || events!.length === 0) {
-    validatedEvents = [
+    events = [
       ListenEvent.added,
       ListenEvent.removed,
       ListenEvent.changed,
       ListenEvent.moved
     ];
   }
-  return validatedEvents;
+  return events;
 }

packages/rxfire/test/database.test.ts

@@ -41,8 +41,8 @@ let batch = (items: any[]) => {
 };
 
 describe('RxFire Database', () => {
-  let app: app.App | null = null;
-  let database: database.Database | null = null;
+  let app: app.App;
+  let database: database.Database;
   let ref = (path: string) => {
     app!.database().goOffline();
     return app!.database().ref(path);
@@ -85,7 +85,7 @@ describe('RxFire Database', () => {
   });
 
   afterEach((done: MochaDone) => {
-    app && app.delete().then(() => done());
+    app.delete().then(() => done());
   });
 
   describe('fromRef', () => {
@@ -396,7 +396,7 @@ describe('RxFire Database', () => {
             expect(data.length).to.eql(items.length - 1);
           })
           .add(done);
-        app && app.database().goOnline();
+        app.database().goOnline();
         aref.set(itemsObj).then(() => {
           aref.child(items[0].key).remove();
         });
@@ -419,7 +419,7 @@ describe('RxFire Database', () => {
             expect(data[1].name).to.eql('lol');
           })
           .add(done);
-        app && app.database().goOnline();
+        app.database().goOnline();
         aref.set(itemsObj).then(() => {
           aref.child(items[1].key).update({ name: 'lol' });
         });
@@ -444,7 +444,7 @@ describe('RxFire Database', () => {
             expect(data[data.length - 1]).to.eql(items[0]);
           })
           .add(done);
-        app && app.database().goOnline();
+        app.database().goOnline();
         aref.set(itemsObj).then(() => {
           aref.child(items[0].key).setPriority('a', () => {});
         });
@@ -542,7 +542,7 @@ describe('RxFire Database', () => {
             expect(data).to.eql(copy);
           })
           .add(done);
-        app && app.database().goOnline();
+        app.database().goOnline();
         ref.set(itemsObj).then(() => {
           ref.child(items[0].key).update({ name });
         });

packages/rxfire/test/firestore.test.ts

@@ -67,8 +67,8 @@ const seedTest = firestore => {
 };
 
 describe('RxFire Firestore', () => {
-  let app: app.App | null = null;
-  let firestore: firestore.Firestore | null = null;
+  let app: app.App;
+  let firestore: firestore.Firestore;
 
   /**
    * Each test runs inside it's own app instance and the app
@@ -88,7 +88,7 @@ describe('RxFire Firestore', () => {
   });
 
   afterEach((done: MochaDone) => {
-    app && app.delete().then(() => done());
+    app.delete().then(() => done());
   });
 
   describe('collection', () => {

packages/storage/src/implementation/metadata.ts

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

packages/storage/src/implementation/observer.ts

@@ -23,9 +23,9 @@ type CompleteFn = () => void;
 type Unsubscribe = () => void;
 
 type Subscribe<T> = (
-  next: NextFn<T> | { [name: string]: string | null } | null | undefined,
-  error: ErrorFn | null | undefined,
-  complete: CompleteFn | null | undefined
+  next?: NextFn<T> | { [name: string]: string | null } | null,
+  error?: ErrorFn | null,
+  complete?: CompleteFn | null
 ) => Unsubscribe;
 
 export { NextFn, ErrorFn, CompleteFn, Unsubscribe, Subscribe };
@@ -34,17 +34,16 @@ export { NextFn, ErrorFn, CompleteFn, Unsubscribe, Subscribe };
  * @struct
  */
 export class Observer<T> {
-  next: NextFn<T> | null;
-  error: ErrorFn | null | undefined;
-  complete: CompleteFn | null;
+  next?: NextFn<T> | null;
+  error?: ErrorFn | null;
+  complete?: CompleteFn | null;
 
   constructor(
-    nextOrObserver:
+    nextOrObserver?:
       | NextFn<T>
       | { [name: string]: string | null }
-      | null
-      | undefined,
-    opt_error?: ErrorFn | null | undefined,
+      | null,
+    opt_error?: ErrorFn | null,
     opt_complete?: CompleteFn | null
   ) {
     let asFunctions =

packages/storage/src/implementation/requests.ts

@@ -271,7 +271,7 @@ export function multipartUpload(
   if (body === null) {
     throw errorsExports.cannotSliceBlob();
   }
-  let urlParams: UrlParams = { name: metadata['fullPath'] || '' };
+  let urlParams: UrlParams = { name: metadata['fullPath']! };
   let url = UrlUtils.makeUrl(urlPart);
   let method = 'POST';
   let timeout = authWrapper.maxUploadRetryTime();

packages/storage/src/reference.ts

@@ -175,7 +175,7 @@ export class Reference {
     let data = fbsString.dataFromString(format, string);
     let metadata = object.clone<Metadata>(opt_metadata);
     if (!type.isDef(metadata['contentType']) && type.isDef(data.contentType)) {
-      metadata['contentType'] = data.contentType || undefined;
+      metadata['contentType'] = data.contentType!;
     }
     return new UploadTask(
       this,

packages/storage/src/task.ts

@@ -455,13 +455,12 @@ export class UploadTask {
    */
   on(
     type: TaskEvent,
-    nextOrObserver:
+    nextOrObserver?:
       | NextFn<UploadTaskSnapshot>
       | { [name: string]: any }
-      | null
-      | undefined = undefined,
-    error: ErrorFn | null | undefined = undefined,
-    completed: CompleteFn | null | undefined = undefined
+      | null,
+    error?: ErrorFn | null,
+    completed?: CompleteFn | null
   ): Unsubscribe | Subscribe<UploadTaskSnapshot> {
     function typeValidator(_p: any) {
       if (type !== TaskEvent.STATE_CHANGED) {
@@ -509,10 +508,9 @@ export class UploadTask {
         nextOrObserver:
           | NextFn<UploadTaskSnapshot>
           | { [name: string]: string | null }
-          | null
-          | undefined,
-        error?: ErrorFn | null | undefined,
-        opt_complete?: CompleteFn | null | undefined
+          | null,
+        error?: ErrorFn | null,
+        opt_complete?: CompleteFn | null
       ) {
         if (specs !== null) {
           fbsArgs.validate('on', specs, arguments);
@@ -627,12 +625,12 @@ export class UploadTask {
     switch (externalState) {
       case TaskState.RUNNING:
       case TaskState.PAUSED:
-        if (observer.next !== null) {
+        if (observer.next) {
           fbsAsync(observer.next.bind(observer, this.snapshot))();
         }
         break;
       case TaskState.SUCCESS:
-        if (observer.complete !== null) {
+        if (observer.complete) {
           fbsAsync(observer.complete.bind(observer))();
         }
         break;

packages/storage/test/blob.test.ts

@@ -41,19 +41,17 @@ describe('Firebase Storage > Blob', () => {
 
   it('Slicing works', () => {
     const blob = new FbsBlob(new Uint8Array([1, 2, 3, 4, 5, 6, 7]));
-    const sliced = blob.slice(1, 5);
-    assert.isNotNull(sliced);
+    const sliced = blob.slice(1, 5)!;
     testShared.assertUint8ArrayEquals(
-      sliced!.uploadData() as Uint8Array,
+      sliced.uploadData() as Uint8Array,
       new Uint8Array([2, 3, 4, 5])
     );
   });
   it('Blobs are merged with strings correctly', () => {
     const blob = new FbsBlob(new Uint8Array([1, 2, 3, 4]));
-    const merged = FbsBlob.getBlob('what', blob, '\ud83d\ude0a ');
-    assert.isNotNull(merged);
+    const merged = FbsBlob.getBlob('what', blob, '\ud83d\ude0a ')!;
     testShared.assertUint8ArrayEquals(
-      merged!.uploadData() as Uint8Array,
+      merged.uploadData() as Uint8Array,
       new Uint8Array([
         0x77,
         0x68,

packages/storage/test/requests.test.ts

@@ -298,7 +298,7 @@ describe('Firebase Storage > Requests', () => {
       [locationNormal, locationNormalNoObjUrl],
       [locationEscapes, locationEscapesNoObjUrl]
     ];
-    const promises: Promise<void>[] = [];
+    const promises: Array<Promise<void>> = [];
     for (let i = 0; i < maps.length; i++) {
       const location = maps[i][0] as Location;
       const url = maps[i][1] as string;

packages/util/src/crypt.ts

@@ -54,7 +54,7 @@ const stringToByteArray = function(str) {
  */
 const byteArrayToString = function(bytes) {
   // TODO(user): Use native implementations if/when available
-  var out: Array<string> = [],
+  var out: string[] = [],
     pos = 0,
     c = 0;
   while (pos < bytes.length) {

packages/util/test/errors.test.ts

@@ -95,7 +95,7 @@ describe('FirebaseError', () => {
     const e = ERROR_FACTORY.create('generic-error');
     assert.isDefined(e.stack);
     // Multi-line match trick - .* does not match \n
-    assert.match(e.stack as string, /FirebaseError[\s\S]/);
+    assert.match(e.stack!, /FirebaseError[\s\S]/);
   });
 
   it('has function names in stack trace in correct order', () => {