PolymerLabs / arcs

Arcs
BSD 3-Clause "New" or "Revised" License
56 stars 35 forks source link

firebase-storage uses nullable version #1823

Open lindner opened 5 years ago

lindner commented 5 years ago

firebase-storage uses a null version to signify special processing. However all of the usages of version are not null checked. This needs to be fixed to allow for strictNullChecks. If the version is changed to non-nullable in storage-provider-base only the following error is emitted:

runtime/ts/storage/firebase-storage.ts:332:5 - error TS2322: Type 'null' is not assignable \to type 'number'.
332     this.version = null;
        ~~~~~~~~~~~~

However if version is allowed to be null then there are about 30-40 instances of version use that are not null checked. Like this:

runtime/ts/storage/in-memory-storage.ts:463:5 - error TS2531: Object is possibly 'null'.

463     this.version++;
        ~~~~~~~~~~~~

One possible solution is to handle uninitialized versions as a special case in firebase-storage as a separate state (perhaps an enum?)

shans commented 5 years ago

Another way of thinking of this is that we need to audit the 30-40 instances of version and ensure that they can only run when version isn't null. However I'm guessing that TypeScript isn't all that great about reasoning about types in general - for example, does this pattern cause TS2531?

assert(this.version !== null);
this.version++;

How about this one?

this.version = data.version; // assuming data.version is typed as Number
this.version++;
shans commented 5 years ago

In the former case, we can provide a type guard:

function foo(version: number | null): number {
    if (version === null)
        throw Error;
    return version++;
}

works just fine.

Unfortunately we can't assign the never type to a more high-level assert function for better semantics:

function fail(message: string) : never {
    throw new Error(message);
}

function foo(version: number | null): number {
    if (version === null)
        fail("version should not be null");
    return version++;
           ^^^^^^^ Object is possibly 'null'.
}
shans commented 5 years ago

The latter case works fine:

function foo(version: number | null, boo: {bar : number}): number {
    version = boo.bar;
    return version++;
}