Apply strictNullChecks to all packages except database #1805

hsubox76 · 2019-05-20T21:55:23Z

Applying Typescript option strictNullChecks to all packages and fixing existing code where it violates this rule.

Database has an override on this for now because it has so many strict null check violations to fix that it should be a separate PR.

packages/storage/src/task.ts

…into ch-strict

jamesdaniels

LGTM rxfire

yuchenshi

LGTM for packages/testing

packages/storage/src/reference.ts

packages/storage/src/task.ts

packages/app/src/firebaseApp.ts

packages/app/test/firebaseApp.test.ts

packages/logger/test/logger.test.ts

packages/rxfire/database/list/index.ts

packages/storage/test/requests.test.ts

packages/storage/test/testshared.ts

packages/util/src/crypt.ts

packages/util/src/obj.ts

packages/util/test/errors.test.ts

packages/app/src/firebaseApp.ts

packages/rxfire/database/utils.ts

packages/app/src/lite/firebaseAppLite.ts

googlebot · 2019-05-24T22:34:52Z

So there's good news and bad news.

👍 The good news is that everyone that needs to sign a CLA (the pull request submitter and all commit authors) have done so. Everything is all good there.

😕 The bad news is that it appears that one or more commits were authored or co-authored by someone other than the pull request submitter. We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that here in the pull request.

Note to project maintainer: This is a terminal state, meaning the cla/google commit status will not change from this state. It's up to you to confirm consent of all the commit author(s), set the cla label to yes (if enabled on your project), and then merge this pull request when appropriate.

ℹ️ Googlers: Go here for more info.

googlebot · 2019-05-25T01:16:07Z

CLAs look good, thanks!

ℹ️ Googlers: Go here for more info.

schmidt-sebastian

One question remaining about the Storage tests but LGTM. Thanks.

packages/storage/test/service.test.ts

packages/storage/src/task.ts

…into ch-strict

Feiyang1 · 2019-05-31T18:31:47Z

packages/functions/src/api/service.ts

@@ -110,7 +110,7 @@ export class Service implements FirebaseFunctions, FirebaseService {
   * @param origin The origin of the local emulator, such as
   * "http://localhost:5005".
   */
-  useFunctionsEmulator(origin: string) {
+  useFunctionsEmulator(origin: string | null) {


Don't think this function is intended to reset the emulator. Plus it's an API change. I would just leave it to only accept string.

Ok, got rid of the test line that uses null to reset this. Doesn't seem necessary.

Feiyang1 · 2019-05-31T18:36:03Z

packages/rxfire/test/database.test.ts

@@ -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 || '']);


could it happen? Should it be snapshot.key! ?

Theoretically it could be null but if it ever happens I think it's so far back in the stack that it's beyond the scope of this test to check for it. Changed to snapshot.key!

Feiyang1 · 2019-05-31T18:40:40Z

packages/storage/src/implementation/list.ts

@@ -58,11 +59,15 @@ function fromBackendResponse(
    items: [],
    nextPageToken: resource['nextPageToken']
  };
+  const bucket = authWrapper.bucket();
+  if (bucket === null) {


This is why I don't like mixing ==null in the code. I have to look up authWrapper.bucket() signature to understand if it is actually meant to be == and made a mistake.

Feiyang1 · 2019-05-31T18:45:07Z

packages/storage/src/reference.ts

@@ -226,7 +226,7 @@ export class Reference {
      prefixes: [],
      items: []
    };
-    return this.listAllHelper(accumulator, null).then(() => accumulator);
+    return this.listAllHelper(accumulator, undefined).then(() => accumulator);


You can just omit the second param

Feiyang1 · 2019-05-31T18:51:09Z

packages/storage/test/service.test.ts

@@ -111,7 +112,7 @@ describe('Firebase Storage > Service', () => {
      const error = testShared.assertThrows(() => {
        new Service(testShared.fakeApp, xhrIoPool, 'gs://bucket/object/');
      }, 'storage/invalid-default-bucket');
-      assert.match(error.message, /Invalid default bucket/);
+      error && assert.match(error.message, /Invalid default bucket/);


assert.match() should always execute. testShared.assertThrows() returns a FirebaseStorageError, I don't think we need to null check error.

Feiyang1 · 2019-05-31T18:51:56Z

packages/storage/test/service.test.ts

@@ -135,7 +136,7 @@ describe('Firebase Storage > Service', () => {
      const error = testShared.assertThrows(() => {
        service.refFromURL('path/to/child');
      }, 'storage/invalid-argument');
-      assert.match(error.message, /invalid/i);
+      error && assert.match(error.message, /invalid/i);


The same as above.

Feiyang1 · 2019-05-31T18:55:38Z

packages/util/src/crypt.ts

@@ -49,12 +49,12 @@ const stringToByteArray = function(str) {
 /**
 * Turns an array of numbers into the string given by the concatenation of the
 * characters to which the numbers correspond.
- * @param {Array<number>} bytes Array of numbers representing characters.
+ * @param {number[]} bytes Array of numbers representing characters.


all the param/return types should be moved from the comment to the definition directly.

Done, fixed similar in rest of file.

…into ch-strict

hsubox76 added 5 commits May 10, 2019 15:54

TS changes before splitting database

21e14c0

Roll back database changes

9f38d85

[AUTOMATED]: Prettier Code Styling

10fe94e

Put back database nullcheck exception

e9d9a93

Finish fixing strictNullCheck errors

263cd24

hsubox76 requested review from davideast, Feiyang1, hiranya911, jamesdaniels, jsdt, mikelehen, ryanpbrewster, schmidt-sebastian, scottcrossen and yuchenshi as code owners May 20, 2019 21:55

schmidt-sebastian reviewed May 20, 2019

View reviewed changes

packages/storage/src/task.ts Outdated Show resolved Hide resolved

hsubox76 added 2 commits May 20, 2019 15:28

Fix logger test.

54bdc4e

Merge branch 'master' of https://github.com/firebase/firebase-js-sdk …

e1ab520

…into ch-strict

jamesdaniels approved these changes May 20, 2019

View reviewed changes

yuchenshi approved these changes May 20, 2019

View reviewed changes

Feiyang1 reviewed May 21, 2019

View reviewed changes

packages/storage/src/reference.ts Outdated Show resolved Hide resolved

packages/storage/src/task.ts Outdated Show resolved Hide resolved

hsubox76 added 4 commits May 21, 2019 15:18

Fix tests

f246e21

Update primitive array types to type[]

3e1ea9b

[AUTOMATED]: Prettier Code Styling

8025369

Put test exclusion back in rxfire

6e668c0

mmermerkaya suggested changes May 22, 2019

View reviewed changes

hsubox76 added 2 commits May 22, 2019 11:32

Address PR comments

ce0118b

[AUTOMATED]: Prettier Code Styling

2e41a2f

mmermerkaya reviewed May 22, 2019

View reviewed changes

packages/app/src/firebaseApp.ts Outdated Show resolved Hide resolved

packages/rxfire/database/utils.ts Outdated Show resolved Hide resolved

mmermerkaya reviewed May 22, 2019

View reviewed changes

packages/app/src/lite/firebaseAppLite.ts Outdated Show resolved Hide resolved

hsubox76 requested review from bojeil-google, dwoffinden, pinarx, wti806 and zijianjoy as code owners May 24, 2019 22:30

wilhuff force-pushed the ch-strict branch from dde0757 to 1c5b5c5 Compare May 25, 2019 01:16

zijianjoy removed request for dwoffinden, andirayo, alikn, bojeil-google, pinarx, wti806 and zijianjoy May 25, 2019 01:33

schmidt-sebastian approved these changes May 25, 2019

View reviewed changes

packages/storage/test/service.test.ts Outdated Show resolved Hide resolved

packages/storage/src/task.ts Outdated Show resolved Hide resolved

schmidt-sebastian assigned hsubox76 May 25, 2019

hsubox76 added 6 commits May 28, 2019 10:22

change assertThrows to assume it never returns null

9c3d52f

[AUTOMATED]: Prettier Code Styling

6ec6881

Merge branch 'master' of https://github.com/firebase/firebase-js-sdk …

384c580

…into ch-strict

Fix additional warnings after merge

aca9dd8

Fix storage test

88f27cf

[AUTOMATED]: Prettier Code Styling

d9c9040

Feiyang1 suggested changes May 31, 2019

View reviewed changes

hsubox76 added 2 commits May 31, 2019 15:00

Address comments in PR

846f100

Merge branch 'master' of https://github.com/firebase/firebase-js-sdk …

df401be

…into ch-strict

Feiyang1 approved these changes Jun 3, 2019

View reviewed changes

hsubox76 merged commit 6668b59 into master Jun 3, 2019

hsubox76 deleted the ch-strict branch June 5, 2019 17:11

firebase locked and limited conversation to collaborators Oct 11, 2019

Apply strictNullChecks to all packages except database #1805

Apply strictNullChecks to all packages except database #1805

Uh oh!

Conversation

hsubox76 commented May 20, 2019

Uh oh!

Uh oh!

jamesdaniels left a comment

Choose a reason for hiding this comment

Uh oh!

yuchenshi left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

googlebot commented May 24, 2019

Uh oh!

googlebot commented May 25, 2019

Uh oh!

schmidt-sebastian left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!