Skip to content
This repository has been archived by the owner on Apr 15, 2019. It is now read-only.

Update utils to typescript - Closes #653 #664

Merged
merged 8 commits into from
Dec 3, 2018

Conversation

shuse2
Copy link
Contributor

@shuse2 shuse2 commented Nov 20, 2018

Description

Updated utils folder into typescript.
This will fail until lisk-elements 2.0 is released.

Review checklist

@shuse2 shuse2 self-assigned this Nov 20, 2018
Copy link
Contributor

@ishantiw ishantiw left a comment

Choose a reason for hiding this comment

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

Missing new lines at the end of some files, if we are following that standard. Else, everything looks good.

src/utils/api.ts Outdated

export default getAPIClient;
return new APIClient(clientNodes, { nethash });
};
Copy link
Contributor

Choose a reason for hiding this comment

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

new line at the end?

"@types/sinon": "5.0.5",
"@types/sinon-chai": "3.2.0",
"@types/strip-ansi": "3.0.0",
"@oclif/dev-cli": "1.18.0",
Copy link
Contributor

Choose a reason for hiding this comment

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

shouldn't @oclif dependencies come before @types?

const ERROR_DATA_MISSING = 'No data was provided.';
const ERROR_DATA_SOURCE = 'Unknown data source type.';
const DEFAULT_TIMEOUT = 100;

export const splitSource = source => {
interface SplittedSource {
Copy link
Contributor

Choose a reason for hiding this comment

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

prefer SplitSource

export const getPassphrase = async (
passphraseSource: string,
options: object,
): Promise<string | ReadonlyArray<string>> => {
Copy link
Contributor

Choose a reason for hiding this comment

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

do we need | ReadonlyArray<string>

// Get endpoints with 2xx status code should always return with data key.
if (!res.data) {
throw new Error('No data was returned.');
}
if (Array.isArray(res.data)) {
if (isArray(res.data)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

why not regular Array.isArray?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Type detection didn't work well here...

@@ -13,8 +13,8 @@
* Removal or modification of this copyright notice is prohibited.
*
*/
export const createFakeInterface = value => ({
on: (type, callback) => {
export const createFakeInterface = (value: any) => ({
Copy link
Contributor

Choose a reason for hiding this comment

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

do we need tslint disable no-any here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

For test, it seems like any is working =0

@@ -51,7 +64,11 @@ describe('query utils', () => {
get: sandbox.stub().resolves(response),
},
};
queryResult = query(apiClient, defaultEndpoint, defaultParameters);
queryResult = query(
(apiClient as unknown) as APIClient,
Copy link
Contributor

Choose a reason for hiding this comment

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

unknown needed here?

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 needed to convert to unknown first otherwise it complains...

return res.data;
};

export const query = async (client, endpoint, parameters) =>
Array.isArray(parameters)
interface QueryParamter {
Copy link
Contributor

Choose a reason for hiding this comment

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

QueryParamter --> QueryParameter

@shuse2 shuse2 changed the base branch from 652-update_to_typescript_setting to 649-convert_to_typescript November 28, 2018 09:39
package.json Outdated
@@ -104,15 +104,15 @@
"/default_config.json"
],
"dependencies": {
"@oclif/command": "1.5.0",
Copy link
Contributor

Choose a reason for hiding this comment

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

why @oclif dependencies were moved up?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

probably npm? but fixed.

@@ -13,7 +13,7 @@
* Removal or modification of this copyright notice is prohibited.
*
*/
import stripANSI from 'strip-ansi';
import strip_ansi from 'strip-ansi';
Copy link
Contributor

Choose a reason for hiding this comment

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

why changed to snake_case?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

this was tslint rule, strip-ansi is exposing function, and it should be used as it's named.

@@ -82,7 +82,9 @@ export const getStdIn = ({
dataIsRequired
)
) {
return resolve({});
resolve({});
Copy link
Contributor

Choose a reason for hiding this comment

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

return resolve({}) was fine I guess or was there any reason?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

tslint rule, where if void is return type, we shouldn't return value, and explicitly do return in new line

@shuse2 shuse2 merged commit 001fd89 into 649-convert_to_typescript Dec 3, 2018
@shuse2 shuse2 deleted the 653-update_utils_to_typescript branch December 13, 2018 13:32
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants