Skip to content

Enable Typescript strict flag for Storage #1935

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 4 commits into from
Jul 8, 2019
Merged

Enable Typescript strict flag for Storage #1935

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
e80d279
Address issues after enabling Typescript strict option
hsubox76 Jul 1, 2019
cfbf20e
[AUTOMATED]: Prettier Code Styling
hsubox76 Jul 1, 2019
6049842
Address PR comments
hsubox76 Jul 2, 2019
ac96379
Add string type check.
hsubox76 Jul 3, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 11 additions & 5 deletions packages/storage/src/implementation/backoff.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -49,10 +49,13 @@ export function start(
}
let triggeredCallback = false;

function triggerCallback(): void {
// TODO: This disable can be removed and the 'ignoreRestArgs' option added to
// the no-explicit-any rule when ESlint releases it.
// eslint-disable-next-line @typescript-eslint/no-explicit-any
function triggerCallback(...args: any[]): void {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we follow the same pattern in handler() and change the signature as well? Something like:

function handler(success:boolean, ...args:any[])

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

if (!triggeredCallback) {
triggeredCallback = true;
callback.apply(null, arguments);
callback.apply(null, args);
}
}

Expand All @@ -63,17 +66,20 @@ export function start(
}, millis);
}

function handler(success: boolean): void {
// TODO: This disable can be removed and the 'ignoreRestArgs' option added to
// the no-explicit-any rule when ESlint releases it.
// eslint-disable-next-line @typescript-eslint/no-explicit-any
function handler(success: boolean, ...args: any[]): void {
if (triggeredCallback) {
return;
}
if (success) {
triggerCallback.apply(null, arguments);
triggerCallback.call(null, success, ...args);
return;
}
const mustStop = canceled() || hitTimeout;
if (mustStop) {
triggerCallback.apply(null, arguments);
triggerCallback.call(null, success, ...args);
return;
}
if (waitSeconds < 64) {
Expand Down
2 changes: 1 addition & 1 deletion packages/storage/src/implementation/blob.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,7 @@ import * as type from './type';
* modified after this blob's construction.
*/
export class FbsBlob {
private data_: Blob | Uint8Array;
private data_!: Blob | Uint8Array;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you explain to me what the bang here means?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is a way of us promising the compiler that this variable will be initialized before anything calls it. The compiler thinks data_ isn't definitively set in the constructor because there's an if and 2 else ifs where it is set, but no else. Since the constructor requires the data param to be of a type that would meet all possible if conditions, it should always get set in the constructor.

I can fix it by adding an else at the end that assigns data_ some default value, which seems risky unless you have a suggestion, or I can just look at the types and promise the compiler data_ is going to get set. Thoughts?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Interesting. I have never seen this before. We have a bunch of code in Firestore that we could simplify this way. So keep it as is please :)

private size_: number;
private type_: string;

Expand Down
43 changes: 23 additions & 20 deletions packages/storage/src/implementation/metadata.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,20 +38,20 @@ export function noXform_<T>(metadata: Metadata, value: T): T {
class Mapping<T> {
local: string;
writable: boolean;
xform: (p1: Metadata, p2: T | undefined) => T | undefined;
xform: (p1: Metadata, p2?: T) => T | undefined;

constructor(
public server: string,
local?: string | null,
writable?: boolean,
xform?: ((p1: Metadata, p2: T | undefined) => T | undefined) | null
xform?: ((p1: Metadata, p2?: T) => T | undefined) | null
) {
this.local = local || server;
this.writable = !!writable;
this.xform = xform || noXform_;
}
}
type Mappings = Array<Mapping<unknown>>;
type Mappings = Array<Mapping<string> | Mapping<number>>;

export { Mappings };

Expand All @@ -69,15 +69,15 @@ export function getMappings(): Mappings {
if (mappings_) {
return mappings_;
}
const mappings: Array<Mapping<unknown>> = [];
mappings.push(new Mapping('bucket'));
mappings.push(new Mapping('generation'));
mappings.push(new Mapping('metageneration'));
mappings.push(new Mapping('name', 'fullPath', true));
const mappings: Mappings = [];
mappings.push(new Mapping<string>('bucket'));
mappings.push(new Mapping<string>('generation'));
mappings.push(new Mapping<string>('metageneration'));
mappings.push(new Mapping<string>('name', 'fullPath', true));

function mappingsXformPath(
_metadata: Metadata,
fullPath: string
fullPath: string | undefined
): string | undefined {
return xformPath(fullPath);
}
Expand All @@ -98,18 +98,18 @@ export function getMappings(): Mappings {
return size;
}
}
const sizeMapping = new Mapping('size');
const sizeMapping = new Mapping<number>('size');
sizeMapping.xform = xformSize;
mappings.push(sizeMapping);
mappings.push(new Mapping('timeCreated'));
mappings.push(new Mapping('updated'));
mappings.push(new Mapping('md5Hash', null, true));
mappings.push(new Mapping('cacheControl', null, true));
mappings.push(new Mapping('contentDisposition', null, true));
mappings.push(new Mapping('contentEncoding', null, true));
mappings.push(new Mapping('contentLanguage', null, true));
mappings.push(new Mapping('contentType', null, true));
mappings.push(new Mapping('metadata', 'customMetadata', true));
mappings.push(new Mapping<number>('timeCreated'));
mappings.push(new Mapping<string>('updated'));
mappings.push(new Mapping<string>('md5Hash', null, true));
mappings.push(new Mapping<string>('cacheControl', null, true));
mappings.push(new Mapping<string>('contentDisposition', null, true));
mappings.push(new Mapping<string>('contentEncoding', null, true));
mappings.push(new Mapping<string>('contentLanguage', null, true));
mappings.push(new Mapping<string>('contentType', null, true));
mappings.push(new Mapping<string>('metadata', 'customMetadata', true));
mappings_ = mappings;
return mappings_;
}
Expand All @@ -134,7 +134,10 @@ export function fromResource(
const len = mappings.length;
for (let i = 0; i < len; i++) {
const mapping = mappings[i];
metadata[mapping.local] = mapping.xform(metadata, resource[mapping.server]);
metadata[mapping.local] = (mapping as Mapping<unknown>).xform(
metadata,
resource[mapping.server]
);
}
addRef(metadata, authWrapper);
return metadata;
Expand Down
2 changes: 1 addition & 1 deletion packages/storage/src/implementation/string.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,7 @@ export const StringFormat = {
DATA_URL: 'data_url'
};

export function formatValidator(stringFormat: string): void {
export function formatValidator(stringFormat: unknown): void {
switch (stringFormat) {
case StringFormat.RAW:
case StringFormat.BASE64:
Expand Down
4 changes: 2 additions & 2 deletions packages/storage/src/implementation/xhrio.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -45,9 +45,9 @@ export interface XhrIo {

getResponseHeader(header: string): string | null;

addUploadProgressListener(listener: (p1: Event) => void): void;
addUploadProgressListener(listener: (p1: ProgressEvent) => void): void;

removeUploadProgressListener(listener: (p1: Event) => void): void;
removeUploadProgressListener(listener: (p1: ProgressEvent) => void): void;
}

/**
Expand Down
4 changes: 2 additions & 2 deletions packages/storage/src/implementation/xhrio_network.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -131,7 +131,7 @@ export class NetworkXhrIo implements XhrIo {
/**
* @override
*/
addUploadProgressListener(listener: (p1: Event) => void): void {
addUploadProgressListener(listener: (p1: ProgressEvent) => void): void {
if (type.isDef(this.xhr_.upload)) {
this.xhr_.upload.addEventListener('progress', listener);
}
Expand All @@ -140,7 +140,7 @@ export class NetworkXhrIo implements XhrIo {
/**
* @override
*/
removeUploadProgressListener(listener: (p1: Event) => void): void {
removeUploadProgressListener(listener: (p1: ProgressEvent) => void): void {
if (type.isDef(this.xhr_.upload)) {
this.xhr_.upload.removeEventListener('progress', listener);
}
Expand Down
16 changes: 11 additions & 5 deletions packages/storage/src/service.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -63,8 +63,11 @@ export class Service {
* bucket.
*/
ref(path?: string): Reference {
function validator(path: string): void {
if (/^[A-Za-z]+:\/\//.test(path)) {
function validator(path: unknown): void {
if (typeof path !== 'string') {
throw 'Path is not a string.';
}
if (/^[A-Za-z]+:\/\//.test(path as string)) {
Copy link
Contributor

@schmidt-sebastian schmidt-sebastian Jul 2, 2019 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you want to go the extra mile, then I would suggest we also validate explicitly that path is a string. Optional.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will give it a shot.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you do the same thing for refFromURL below? Thanks!

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

throw 'Expected child path but got a URL, use refFromURL instead.';
}
}
Expand All @@ -86,12 +89,15 @@ export class Service {
* which must be a gs:// or http[s]:// URL.
*/
refFromURL(url: string): Reference {
function validator(p: string): void {
if (!/^[A-Za-z]+:\/\//.test(p)) {
function validator(p: unknown): void {
if (typeof p !== 'string') {
throw 'Path is not a string.';
}
if (!/^[A-Za-z]+:\/\//.test(p as string)) {
throw 'Expected full URL but got a child path, use ref instead.';
}
try {
Location.makeFromUrl(p);
Location.makeFromUrl(p as string);
} catch (e) {
throw 'Expected valid full URL but got an invalid one.';
}
Expand Down
6 changes: 3 additions & 3 deletions packages/storage/src/task.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -507,7 +507,7 @@ export class UploadTask {
specs: ArgSpec[] | null
): Subscribe<UploadTaskSnapshot> {
function binder(
nextOrObserver:
nextOrObserver?:
| NextFn<UploadTaskSnapshot>
| StorageObserver<UploadTaskSnapshot>
| null,
Expand All @@ -526,7 +526,7 @@ export class UploadTask {
return binder;
}

function binderNextOrObserverValidator(p: {}): void {
function binderNextOrObserverValidator(p: unknown): void {
if (p === null) {
throw nextOrObserverMessage;
}
Expand Down Expand Up @@ -557,7 +557,7 @@ export class UploadTask {
*/
then<U>(
onFulfilled?: ((value: UploadTaskSnapshot) => U | Promise<U>) | null,
onRejected?: ((error: unknown) => U | Promise<U>) | null
onRejected?: ((error: Error) => U | Promise<U>) | null
): Promise<U> {
// These casts are needed so that TypeScript can infer the types of the
// resulting Promise.
Expand Down
3 changes: 2 additions & 1 deletion packages/storage/test/reference.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -591,7 +591,8 @@ describe('Firebase Storage > Reference', () => {
});
it('updateMetadata throws', () => {
testShared.assertThrows(
root.updateMetadata.bind(root, {}),
// eslint-disable-next-line @typescript-eslint/no-explicit-any
(root as any).updateMetadata.bind(root, {}),
'storage/invalid-root-operation'
);
});
Expand Down
8 changes: 5 additions & 3 deletions packages/storage/test/request.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,7 +38,7 @@ describe('Firebase Storage > Request', () => {
const response = 'I am the server response!!!!';

function newSend(xhrio: TestingXhrIo): void {
const responseHeaders = {};
const responseHeaders: { [key: string]: string } = {};
responseHeaders[responseHeader] = responseValue;
xhrio.simulateResponse(status, response, responseHeaders);
}
Expand Down Expand Up @@ -68,7 +68,7 @@ describe('Firebase Storage > Request', () => {
const args: unknown[] = spiedSend.getCall(0).args;
assert.equal(args[1], url);
assert.equal(args[2], method);
const expectedHeaders: Headers = {};
const expectedHeaders: { [key: string]: string } = {};
expectedHeaders[requestHeader] = requestValue;
expectedHeaders[versionHeaderName] = versionHeaderValue;
assert.deepEqual(args[4], expectedHeaders);
Expand Down Expand Up @@ -197,7 +197,9 @@ describe('Firebase Storage > Request', () => {
() => {
assert.isTrue(spiedSend.calledOnce);
const args: unknown[] = spiedSend.getCall(0).args;
const expectedHeaders = { Authorization: 'Firebase ' + authToken };
const expectedHeaders: { [key: string]: string } = {
Authorization: 'Firebase ' + authToken
};
expectedHeaders[versionHeaderName] = versionHeaderValue;
assert.deepEqual(args[4], expectedHeaders);
},
Expand Down
7 changes: 5 additions & 2 deletions packages/storage/test/service.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -307,8 +307,11 @@ describe('Firebase Storage > Service', () => {
ref.put(new Blob(['a'])).on(
TaskEvent.STATE_CHANGED,
null,
(err: FirebaseStorageError) => {
assert.equal(err.code, 'storage/app-deleted');
(err: FirebaseStorageError | Error) => {
assert.equal(
(err as FirebaseStorageError).code,
'storage/app-deleted'
);
resolve();
},
() => {
Expand Down
10 changes: 5 additions & 5 deletions packages/storage/test/task.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -45,7 +45,7 @@ interface Response {
type RequestHandler = (
url: string,
method: string,
body?: ArrayBufferView | Blob | string,
body?: ArrayBufferView | Blob | string | null,
headers?: Headers
) => Response;

Expand All @@ -54,7 +54,7 @@ function authWrapperWithHandler(handler: RequestHandler): AuthWrapper {
xhrio: TestingXhrIo,
url: string,
method: string,
body?: ArrayBufferView | Blob | string,
body?: ArrayBufferView | Blob | string | null,
headers?: Headers
): void {
const response = handler(url, method, body, headers);
Expand Down Expand Up @@ -97,7 +97,7 @@ function fakeServerHandler(): RequestHandler {
function handler(
url: string,
method: string,
content?: ArrayBufferView | Blob | string,
content?: ArrayBufferView | Blob | string | null,
headers?: Headers
): Response {
method = method || 'GET';
Expand Down Expand Up @@ -268,7 +268,7 @@ describe('Firebase Storage > Upload Task', () => {

// h3: This one will get executed immediately
(() => {
let lastState;
let lastState: TaskState;
return task.on(
TaskEvent.STATE_CHANGED,
snapshot => {
Expand Down Expand Up @@ -346,7 +346,7 @@ describe('Firebase Storage > Upload Task', () => {
function promiseAssertWrapper<T>(func: T): T {
function wrapped(..._args: any[]): void {
try {
((func as any) as (...args: any[]) => void).apply(null, arguments);
((func as any) as (...args: any[]) => void).apply(null, _args);
} catch (e) {
reject(e);
// also throw to further unwind the stack
Expand Down
8 changes: 6 additions & 2 deletions packages/storage/test/xhrio.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,7 +38,7 @@ export interface StringHeaders {
export class TestingXhrIo implements XhrIo {
private state: State;
private sendPromise: Promise<XhrIo>;
private resolve: (xhrIo: XhrIo) => void;
private resolve!: (xhrIo: XhrIo) => void;
private sendHook: SendHook | null;
private status: number;
private responseText: string;
Expand Down Expand Up @@ -75,7 +75,11 @@ export class TestingXhrIo implements XhrIo {
return this.sendPromise;
}

simulateResponse(status: number, body: string, headers: Headers): void {
simulateResponse(
status: number,
body: string,
headers: { [key: string]: string }
): void {
if (this.state !== State.SENT) {
throw new Error("Can't simulate response before send/more than once");
}
Expand Down
3 changes: 2 additions & 1 deletion packages/storage/tsconfig.json
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,8 @@
{
"extends": "../../config/tsconfig.base.json",
"compilerOptions": {
"outDir": "dist"
"outDir": "dist",
"strict": true
},
"exclude": [
"dist/**/*"
Expand Down