Skip to content
This repository was archived by the owner on Jan 16, 2022. It is now read-only.

Refactor: Enabled typescript strict mode #504

Merged
juanpicado merged 4 commits into from
Jul 3, 2020
Merged

Refactor: Enabled typescript strict mode #504

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
955d42f
refactor: enabled tsc strict mode
tmkn Jun 29, 2020
cfffdc2
chore: upgraded typescript to 3.9.5
tmkn Jun 29, 2020
640287b
Update src/utils/api.test.ts
tmkn Jul 1, 2020
f845712
chore: updated typescript to 3.9.6
tmkn Jul 2, 2020
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
3 changes: 1 addition & 2 deletions package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -118,7 +118,7 @@
"stylelint-webpack-plugin": "1.1.2",
"supertest": "4.0.2",
"typeface-roboto": "0.0.75",
"typescript": "3.7.4",
"typescript": "3.9.5",
"uglifyjs-webpack-plugin": "2.2.0",
"url-loader": "3.0.0",
"validator": "12.1.0",
Expand Down Expand Up @@ -169,7 +169,6 @@
"scripts": {
"type-check": "tsc --noEmit --pretty",
"type-check:watch": "npm run type-check -- --watch",
"type-check-strict:watch": "tsc --project ./tsconfig.strict.json --noEmit --pretty --watch",
"release": "standard-version -a",
"test:clean": "npx jest --clearCache",
"test:acceptance": "codeceptjs run --steps",
Expand Down
6 changes: 3 additions & 3 deletions src/App/Header/Header.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,9 +22,9 @@ interface Props {
const Header: React.FC<Props> = ({ withoutSearch }) => {
const { t } = useTranslation();
const appContext = useContext(AppContext);
const [isInfoDialogOpen, setOpenInfoDialog] = useState();
const [showMobileNavBar, setShowMobileNavBar] = useState();
const [showLoginModal, setShowLoginModal] = useState(false);
const [isInfoDialogOpen, setOpenInfoDialog] = useState<boolean>(false);
Copy link
Copy Markdown
Contributor

@priscilawebdev priscilawebdev Jul 1, 2020

Choose a reason for hiding this comment

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

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

Copy link
Copy Markdown
Contributor Author

@tmkn tmkn Jul 1, 2020

Choose a reason for hiding this comment

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

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 😃

Copy link
Copy Markdown
Member

@juanpicado juanpicado Jul 3, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

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.

Screen Shot 2020-07-03 at 7 57 25 AM

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

Screen Shot 2020-07-03 at 7 57 10 AM

Copy link
Copy Markdown
Member

@juanpicado juanpicado Jul 3, 2020

Choose a reason for hiding this comment

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

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.

Copy link
Copy Markdown
Member

@juanpicado juanpicado Jul 3, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

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

Copy link
Copy Markdown
Contributor Author

@tmkn tmkn Jul 3, 2020

Choose a reason for hiding this comment

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

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.

const [showMobileNavBar, setShowMobileNavBar] = useState<boolean>(false);
const [showLoginModal, setShowLoginModal] = useState<boolean>(false);

if (!appContext) {
throw Error(t('app-context-not-correct-used'));
Expand Down
4 changes: 2 additions & 2 deletions src/App/Header/HeaderRight.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,8 +27,8 @@ const HeaderRight: React.FC<Props> = ({
onOpenRegistryInfoDialog,
}) => {
const themeContext = useContext(ThemeContext);
const [anchorEl, setAnchorEl] = useState();
const [isMenuOpen, setIsMenuOpen] = useState();
const [anchorEl, setAnchorEl] = useState<HTMLElement | null>(null);
const [isMenuOpen, setIsMenuOpen] = useState<boolean>(false);

const { t } = useTranslation();

Expand Down
4 changes: 2 additions & 2 deletions src/App/Header/LoginDialog/LoginDialog.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@ import { useTranslation } from 'react-i18next';

import Dialog from 'verdaccio-ui/components/Dialog';
import DialogContent from 'verdaccio-ui/components/DialogContent';
import { makeLogin } from 'verdaccio-ui/utils/login';
import { makeLogin, LoginError } from 'verdaccio-ui/utils/login';
import storage from 'verdaccio-ui/utils/storage';

import AppContext from '../../../App/AppContext';
Expand All @@ -25,7 +25,7 @@ const LoginDialog: React.FC<Props> = ({ onClose, open = false }) => {
throw Error(t('app-context-not-correct-used'));
}

const [error, setError] = useState();
const [error, setError] = useState<LoginError>();

const handleDoLogin = useCallback(
async (data: FormValues) => {
Expand Down
9 changes: 5 additions & 4 deletions src/pages/Version/VersionContextProvider.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
import React, { useEffect, useState } from 'react';
import { useParams } from 'react-router-dom';
import { PackageMetaInterface } from 'types/packageMeta';

import { callDetailPage, callReadme } from 'verdaccio-ui/utils/calls';

Expand All @@ -17,10 +18,10 @@ const VersionContextProvider: React.FC = ({ children }) => {
const { version, package: pkgName, scope } = useParams<Params>();
const [packageName, setPackageName] = useState(getRouterPackageName(pkgName, scope));
const [packageVersion, setPackageVersion] = useState(version);
const [packageMeta, setPackageMeta] = useState();
const [readMe, setReadme] = useState();
const [isLoading, setIsLoading] = useState(true);
const [hasNotBeenFound, setHasNotBeenFound] = useState();
const [packageMeta, setPackageMeta] = useState<PackageMetaInterface>();
const [readMe, setReadme] = useState<string>();
const [isLoading, setIsLoading] = useState<boolean>(true);
const [hasNotBeenFound, setHasNotBeenFound] = useState<boolean>();

useEffect(() => {
const updatedPackageName = getRouterPackageName(pkgName, scope);
Expand Down
12 changes: 7 additions & 5 deletions src/utils/api.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,16 +3,18 @@ import api, { handleResponseType } from '../../src/utils/api';
describe('api', () => {
describe('handleResponseType', () => {
test('should handle missing Content-Type', async () => {
const responseText = `responseText`;
const ok = false;
Copy link
Copy Markdown
Contributor

@priscilawebdev priscilawebdev Jul 1, 2020

Choose a reason for hiding this comment

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

you could move this line to the object below 😉

Copy link
Copy Markdown
Contributor Author

@tmkn tmkn Jul 1, 2020

Choose a reason for hiding this comment

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

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.

Copy link
Copy Markdown
Member

@juanpicado juanpicado Jul 3, 2020

Choose a reason for hiding this comment

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

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

const response: Response = {
url: 'http://localhost:8080/-/packages',
ok: false,
ok: ok,
headers: new Headers(),
text: async () => responseText,
} as Response;

const handled = await handleResponseType(response);

// Should this actually return [false, null] ?
expect(handled).toBeUndefined();
expect(handled).toEqual([ok, responseText]);
});

test('should test tgz scenario', async () => {
Expand Down Expand Up @@ -94,9 +96,9 @@ describe('api', () => {

expect(fetchSpy).toHaveBeenCalledWith('https://verdaccio.tld/resource', {
credentials: 'same-origin',
headers: {
headers: new Headers({
Authorization: 'Bearer token-xx-xx-xx',
},
}),
method: 'GET',
});
expect(response).toEqual({ c: 3 });
Expand Down
13 changes: 7 additions & 6 deletions src/utils/api.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@ import '../../types';
* @param {object} response
* @returns {promise}
*/
export function handleResponseType(response: Response): Promise<[boolean, Blob | string] | void> {
export function handleResponseType(response: Response): Promise<[boolean, any]> {
Copy link
Copy Markdown
Member

@juanpicado juanpicado Jul 3, 2020

Choose a reason for hiding this comment

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

Just wondering why do we remove types here.

Copy link
Copy Markdown
Contributor Author

@tmkn tmkn Jul 3, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

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.

Copy link
Copy Markdown
Member

@juanpicado juanpicado Jul 3, 2020

Choose a reason for hiding this comment

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

Thanks @tmkn :) well explained.

if (response.headers) {
const contentType = response.headers.get('Content-Type');
if (contentType && contentType.includes('application/pdf')) {
Expand All @@ -26,7 +26,7 @@ export function handleResponseType(response: Response): Promise<[boolean, Blob |
}
}

return Promise.resolve();
return Promise.all([response.ok, response.text()]);
}

class API {
Expand All @@ -36,10 +36,11 @@ class API {
}

const token = storage.getItem('token');
if (token && options.headers && typeof options.headers['Authorization'] === 'undefined') {
options.headers = Object.assign({}, options.headers, {
['Authorization']: `Bearer ${token}`,
});
const headers = new Headers(options.headers);

if (token && headers.has('Authorization') === false) {
headers.set('Authorization', `Bearer ${token}`);
options.headers = headers;
}

if (!['http://', 'https://', '//'].some(prefix => url.startsWith(prefix))) {
Expand Down
8 changes: 4 additions & 4 deletions src/utils/calls.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,15 +2,15 @@ import { PackageMetaInterface } from '../../types/packageMeta';

import API from './api';

export async function callReadme(packageName: string, packageVersion?: string): Promise<string | {}> {
return await API.request<string | {}>(
export async function callReadme(packageName: string, packageVersion?: string): Promise<string> {
return await API.request<string>(
`package/readme/${packageName}${packageVersion ? `?v=${packageVersion}` : ''}`,
'GET'
);
}

export async function callDetailPage(packageName: string, packageVersion?: string): Promise<PackageMetaInterface | {}> {
const packageMeta = await API.request<PackageMetaInterface | {}>(
export async function callDetailPage(packageName: string, packageVersion?: string): Promise<PackageMetaInterface> {
const packageMeta = await API.request<PackageMetaInterface>(
`sidebar/${packageName}${packageVersion ? `?v=${packageVersion}` : ''}`,
'GET'
);
Expand Down
1 change: 0 additions & 1 deletion tsconfig.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,6 @@
"target": "esnext",
"module": "commonjs",
"moduleResolution": "node",
"noImplicitAny": false,
"strict": true,
"outDir": "build",
"allowSyntheticDefaultImports": true,
Expand Down
6 changes: 0 additions & 6 deletions tsconfig.strict.json
View file
Open in desktop

This file was deleted.

8 changes: 4 additions & 4 deletions yarn.lock
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14803,10 +14803,10 @@ typeface-roboto@0.0.75:
resolved "https://registry.verdaccio.org/typeface-roboto/-/typeface-roboto-0.0.75.tgz#98d5ba35ec234bbc7172374c8297277099cc712b"
integrity sha512-VrR/IiH00Z1tFP4vDGfwZ1esNqTiDMchBEXYY9kilT6wRGgFoCAlgkEUMHb1E3mB0FsfZhv756IF0+R+SFPfdg==

typescript@3.7.4:
version "3.7.4"
resolved "https://registry.verdaccio.org/typescript/-/typescript-3.7.4.tgz#1743a5ec5fef6a1fa9f3e4708e33c81c73876c19"
integrity sha512-A25xv5XCtarLwXpcDNZzCGvW2D1S3/bACratYBx2sax8PefsFhlYmkQicKHvpYflFS8if4zne5zT5kpJ7pzuvw==
typescript@3.9.5:
version "3.9.5"
resolved "https://registry.verdaccio.org/typescript/-/typescript-3.9.5.tgz#586f0dba300cde8be52dd1ac4f7e1009c1b13f36"
integrity sha512-hSAifV3k+i6lEoCJ2k6R2Z/rp/H3+8sdmcn5NrS3/3kE7+RyZXm9aqvxWqjEXHAd8b0pShatpcdMTvEdvAJltQ==

uglify-js@3.4.x:
version "3.4.10"
Expand Down
⚡