Upgraded Typescript to 3.7.x

[hiranya911]

New TS compiler flagged a semantic error in api-request.ts which I have fixed here.

[Feiyang1]

What was the ts error?
Can you just do if(this.pollCallback !== undefined)? typeof isn't necessary here.

[hiranya911]

The error was:

src/utils/api-request.ts(936,9): error TS2774: This condition will always return true since the function is always defined. Did you mean to call it instead?
TypeScript: 1 semantic error

And made the suggested change.

[nbegley]

Is it true that the function is always defined? Does the error get fixed if line 915 is changed to private pollCallback: () => Promise<object> = null;?

[hiranya911]

That requires more code changes. We need to declare the type as (() => Promise<object>) | null to be able to assign null. When we do that we get another compiler error from the repoll() method saying Cannot invoke an object which is possibly 'null'.

[nbegley]

I think using if(this.pollCallback !== undefined) while keeping the type the same is just avoiding the compiler error without fixing the underlying issue. We should probably either make that (() => Promise<object>) | null type change, or introduce a separate boolean like this:

private started = false;

public poll(callback: () => Promise<object>): Promise<object> {
  if (this.started) {
    throw ...
  }
  this.started = true;
  ...
}

[Feiyang1]

I see. How about we type it (() => Promise<object>) | null to reflect the accurate typing, so no change here. But in repoll(), we do

// we only call repoll() in poll() after this.pollCallback is assigned.
this.pollCallback!().then(...)

[hiranya911]

But is there an underlying issue (if so what?), or is it just a false positive from the new TS version?

EDIT: As far as I can see TS incorrectly thinks pollCallback is a statically-defined function because it has a function type, whereas it is really just a variable.

[hiranya911]

Was going through the test cases in microsoft/TypeScript@006a327#diff-27ba0012946727ed197bf4c5dc9f9194

Looks like the correct solution is to declare this variable as pollCallback?.

[nbegley]

The underlying issue is that pollCallback's type is telling TS that it can't be null. I'm somewhat surprised that we're not getting an error for giving it that type and not setting it when it's defined or in the constructor. Does TS assume that any variable can be undefined even if it's not specified in its type?

+1 to @Feiyang1's suggestion. I think that's more correct than using a separate bool. And using ? instead of | null is slightly better IMO.

[nbegley]

@hiranya911 yep, sounds good.