Refactor: Enabled typescript strict mode

[tmkn]

This PR removes the "noImplicitAny": false option from tsconfig.json.
Meaning the source code is now compatible with the Typescript strict setting, providing the highest type safety. 🎉

To enable this I had to slightly adapt the ajax code, since it wasn't possible to type the existing code without introducing too many changes.
API.request can return a string, json or a blob based on the response content type or url ending or void if those checks don't match.
This proved to be a quite difficult scenario to type, so as to avoid too many changes to the existing code, API.request now returns a string (the response bodys string) where it would have previously returned undefined. This allowed for only minimal changes to the existing ajax code.

However the type safety in this part is not as good as it could be. e.g:

const response: LoginBody = await API.request('login', ...);

While the code expects a LoginBody, at runtime it could return a Blob since as said the actual return type depends on the responses content type or url ending.
It might make sense to provide separate ajax functions for when to expect string, json or Blob like:

const response: LoginBody = await API.requestJson('login', ...);

This way API.requestJson could throw when it can't return a valid json/finds a Blob. This would make the code more failsafe.
Anyway the current code nevertheless is now strict mode compatible.

I also removed all the temporary helper files that where added to incrementally make strict mode happen.

TypeScript was also updated to the (as of now) latest version: 3.9.5

tldr:

• Set strict mode compilation as the new default
• API.request now returns a string in cases where it would have returned undefined previously
• Updated Typescript from 3.7.4 to 3.9.5

[juanpicado]

Great move :) 👏 ... let's see how material-ui reacts on this.

[priscilawebdev]

sometimes when TypeScript can’t infer the type, it makes sense to pass a generic parameter, but in most of the cases TS can cleverly infer the type for useState, especially when we pass a default value... So I believe that passing a generic parameter here is not really necessary + it's very verbose

[tmkn]

While I agree, in some useState calls they were necessary. So in order to not have diverging coding styles I unified the useState declarations in places that were affected. Also while they not seem helpful right now, because the code works, TypeScript is now able to error when you try to initialize a state with the wrong type thanks to the generic usage, after all this PR is all about increasing the type safety 😃

[juanpicado]

While Typescript infers values, it does based on how is initialized. So without type anyone could change to 123 and will be hard to notice the original author actually wants only boolean.

while the type, perfectly is noticeable and any contributor is aware can only use booleans and not any other value.

[juanpicado]

Anyway, I could not find a eslint rule for this, so, let's use common sense. Let's use types for complex objects while for native let's Typescript do the magic.

[juanpicado]

And I refer to useState. For instance things like Promise<string> or const [packageMeta, setPackageMeta] = useState<PackageMetaInterface>(); make totally sense.

[tmkn]

to add there is a subtle difference in the inferred typings for useState
used like this: useState<boolean>(); the type will be boolean | undefined while
useState<boolean>(true); the type will be just boolean

I just like to be more explicit. e.g.

const getValue = () => true;
const [useFlag, setUseFlag] = useState(getValue());

if getValue returns a string, then useState will not error even though we expect a boolean, it will happily infer the string type and will instead error further down below, while if useState would have made use of the generic to specify the type, it would have error'd.

[priscilawebdev]

you could move this line to the object below 😉

[tmkn]

Yes but I kept them separate because responseText and ok are part of the assertion, so I declared them together at the top, also so that it's obvious what are the input parameters for the unit test.

[juanpicado]

ok is being used on the response object. Cannot be below of it.

[sonarqubecloud]

Kudos, SonarCloud Quality Gate passed!

0 Bugs
0 Vulnerabilities (and 0 Security Hotspots to review)
0 Code Smells

No Coverage information
0.0% Duplication

[juanpicado]

Just wondering why do we remove types here.

[tmkn]

Because it also returns json:

ui/src/utils/api.ts

Line 16 in f845712

return Promise.all([response.ok, response.json()]);

So in this case it returns the tuple [boolean, any] since .json() returns any.

So to be technically correct the return type should be Promise<[boolean, Blob | string | any]>, but if you have a union type like this and at any part there is any, it will collapse to any.

Now you could write it like Promise<[boolean, Blob | string | any]> but then

ui/src/utils/api.ts

Line 59 in f845712

resolve(response[1]);

errors, since the signature for the request method looks like this:

ui/src/utils/api.ts

Line 33 in f845712

public request<T>(url: string, method = 'GET', options: RequestInit = { headers: {} }): Promise<T> {

You would then need a typecast at resolve(response[1] as any); which I tried to avoid or go down a rabbit hole of type errors/potential rewrite of the ajax logic.

As I said the ajax functionality is a little bit cumbersome to type as it is. The main cause being that the return type is determined dynamically at runtime (in the handleResponseType function), thus there is no natural way for TypeScript to tell or narrow down the actual return type without explicit casts for the current code. It would help if there were separate ajax functions for json, blob and text, so TypeScript could provide better type safety.
Because right now, the ajax usage is not very type safe:

ui/src/utils/login.ts

Line 66 in f845712

const response: LoginBody = await API.request('login', 'POST', {

The server could return a string, in which case there is no code to handle the case that it actually returned a string but json was expected.

So in short, without introducing too many type casts/changes to the existing logic, I changed it to any to provide a minimum of type safety with the current layout of the code.

[juanpicado]

Thanks @tmkn :) well explained.

[juanpicado]

LGTM, thanks @tmkn for keep TS updated.

Next time we could allow TS infer more for us thus the contributor write less and the reviewer read less. Win win.

[juanpicado]

I'd qualify this as feat due the changes might be considerable due the typings in the future.