Refactor/mvpn 267 enable tslint #8

mipo47 · 2018-10-08T12:59:50Z

Please ask questions related with TsLint enabling only :)

shroomist · 2018-10-08T13:09:30Z

js/app/app-styles.ts


-interface AppStyles {
+interface IAppStyles {


is this an interface or a type?

?? Why do we need I prefix for interfaces? It just makes name longer and couples name to the fact that it's an Interface - if we change it to class, than we will have to change name as well.

"I" is standard for interface types in most of languages. same is required by default tslint config.

I don't think this is necessary to follow this default rule. thoughts? @donce

Makes it easy to differentiate interfaces from regular classes at a glance.

do we need this?

Styling is very subjective, so it's tricky, but I personally agree that it's not needed.

I.e. in typescript compiler repository "I" prefix is forbidden. I've encountered "I" prefix as a bad example of naming in multiple good-naming materials.

We are already using Standard JS for mysterium-vpn - tslint seems to have it as well:
https://github.com/blakeembrey/tslint-config-standard. I've found this because somebody is complaining about tslint defaults not making sense :D palantir/tslint#4199

js/app/app.tsx

shroomist · 2018-10-08T13:17:08Z

js/app/proposals.tsx

-    )
+  private async onFavoritePress(selectedProviderId: string): Promise<void> {
+    const favoriteProposals = this.props.proposalsStore.FavoriteProposals
+    if (!favoriteProposals) {


no error handling?

FavoriteProposals is not computed, unable to get exception here

We are just silently ignoring an error case here of !favoriteProposals.
why do you think this is good?

js/app/app.tsx

js/app/proposals.tsx

shroomist · 2018-10-08T13:27:06Z

js/fetchers/proposals-fetcher.ts

    this.start(CONFIG.REFRESH_INTERVALS.PROPOSALS)
  }

  @action
-  async fetch (): Promise<FavoriteProposalDTO[]> {
-    const proposals: Array<ProposalDTO> = await this._api.findProposals()
+  protected async fetch(): Promise<FavoriteProposalDTO[]> {


hm. how does this modify the state, I don't see.

TODO: fill be fixed

js/libraries/favorite-proposal.ts

shroomist · 2018-10-08T13:36:15Z

js/fetchers/ip-fetcher.ts

+    | 'Connecting'
+    | null = null
+
+  constructor(api: TequilapiClient) {


IPFetcher doesn't need to know the whole tequila, but just .connectionIP()

yes, but it's easier to give full access instead of extracting interface with connectionIP (less code)

also this way it is more difficult to understand the dependency -> difficult to debug.

TODO: fill be fixed

js/app/proposals.tsx

shroomist · 2018-10-08T13:42:01Z

.prettierrc

@@ -0,0 +1,5 @@
+{


why this is .prettierrc and not tslint.json?

I think it's good to add prettier or .editorconfig settings to project as it simplifies consistent config across team a lot.

js/app/app.tsx

VaidotasSm · 2018-10-08T20:08:58Z

js/app/app-tequilapi.ts

+      }
+    } catch (e) {
+      console.warn('api.identityCreate failed', e)
+      return


I'm not familiar with internals of TequilapiClient and errors it throws (and if we can inspect error object to log specific failure), but I would prefer one try-catch instead of multiple per each operation.

How do you want to change this code?

VaidotasSm · 2018-10-08T20:12:29Z

js/app/proposals.tsx

+
+    const favoriteProposal = favoriteProposals.filter(
+      p => p.id === selectedProviderId
+    )[0]


I think this could fail with zero-length - you could check it above with if (!favoriteProposals || !favoriteProposals.length)

for zero-length favoriteProposal will be equal to undefined without exception

so here, user has not added favoutites before. favoriteProposals.length == 0 would be here right?
favoriteProposal would be undefined, and if (favoriteProposal) would never allow to add a favorite.
??

VaidotasSm · 2018-10-08T20:14:47Z

js/fetchers/fetcher.ts

-      if (JSON.stringify(data) !== JSON.stringify(this._prevData)) {
-        this._prevData = data
-        console.info(`Fetcher '${this._name}' returns`, data)
+      if (JSON.stringify(data) !== JSON.stringify(this.prevData)) {


I would lean on using proper deepEquality util, as maybe in this case it's acceptable, but it would fail if properties are reordered.

VaidotasSm · 2018-10-08T20:17:36Z

.prettierrc

@@ -0,0 +1,5 @@
+{


I think it's good to add prettier or .editorconfig settings to project as it simplifies consistent config across team a lot.

donce · 2018-10-09T10:46:43Z

js/app/app-styles.ts


-interface AppStyles {
+interface IAppStyles {


?? Why do we need I prefix for interfaces? It just makes name longer and couples name to the fact that it's an Interface - if we change it to class, than we will have to change name as well.

js/app/app-tequilapi.ts

js/app/app.tsx

donce · 2018-10-09T16:48:14Z

js/app/app.tsx

    super(props)

    // Bind local functions
    this.connectDisconnect = this.connectDisconnect.bind(this)
  }

+  public render() {


?? It returns value, but it's not visible - is it a common practice for react+typescript to ignore return type for render?

yes, because it can return different types

it is not a common practice.
especially if this returns multiple different values, they Must be specified!
!!

What type should I return?

package.json

js/config.ts

Arrnas

Can't really say much, only thing I noticed were the multiline statements - is there a set char limit for lines? I like to only make multiline statements when a single line goes over limit.

shroomist · 2018-10-11T09:03:05Z

js/app/proposals.tsx

-    )
+  private async onFavoritePress(selectedProviderId: string): Promise<void> {
+    const favoriteProposals = this.props.proposalsStore.FavoriteProposals
+    if (!favoriteProposals) {


We are just silently ignoring an error case here of !favoriteProposals.
why do you think this is good?

js/app/proposals.tsx

shroomist · 2018-10-11T09:08:13Z

js/fetchers/ip-fetcher.ts

+    | 'Connecting'
+    | null = null
+
+  constructor(api: TequilapiClient) {


also this way it is more difficult to understand the dependency -> difficult to debug.

shroomist · 2018-10-11T09:10:27Z

js/fetchers/proposals-fetcher.ts

    store.FavoriteProposals = favoriteProposals

    // ensure that proposal is always selected
-    if (store.FavoriteProposals.length
-      && store.FavoriteProposals.filter(p => p.id == store.SelectedProviderId).length === 0
+    if (


why do we need to ensure this?

Dropdown doesn't support non selected items, so I decide to select first proposals by default

js/fetchers/status-fetcher.ts

shroomist · 2018-10-11T09:20:02Z

js/app/proposals.tsx

+
+    const favoriteProposal = favoriteProposals.filter(
+      p => p.id === selectedProviderId
+    )[0]


so here, user has not added favoutites before. favoriteProposals.length == 0 would be here right?
favoriteProposal would be undefined, and if (favoriteProposal) would never allow to add a favorite.
??

js/app/proposals.tsx

shroomist · 2018-10-11T09:22:15Z

js/app/stats.tsx


-interface StatsProps {
+interface IStatsProps {


I mean IStatsProps does not have function descriptions, so it should be a type not an interface.

js/config.ts

shroomist · 2018-10-11T09:24:49Z

package.json

@@ -32,9 +33,14 @@
    "eslint-plugin-react": "^7.10.0",
    "eslint-plugin-standard": "^3.0.1",
    "jest-expo": "~27.0.0",
+    "prettier": "1.14.3",


?? why do we need extra tool if we're adding tslint?
tslint has the --fix flag

shroomist · 2018-10-16T08:17:50Z

js/app/stats.tsx


-interface StatsProps {
+interface IStatsProps {


Some props are types these are interfaces.
please be consistent

shroomist · 2018-10-16T08:21:09Z

js/fetchers/stats-fetcher.ts

+import { store } from '../store/app-store'
+import { FetcherBase } from './fetcher-base'
+
+export type IStatsFetcherProps = {


!! changed to types but I prefix remains. please fix.

js/libraries/tequilapi/enums.ts

js/libraries/unitConverter.ts

shroomist · 2018-10-16T08:40:58Z

js/store/app-store.ts

-  @observable FavoriteProposals: FavoriteProposalDTO[] | null = null
+class AppStore {
+  @observable
+  public IdentityId: string | null = null


i think public IdentityId: ?string could work the same way with less boilerplate

nice topic to discuss before starting refactoring task

donce · 2018-10-16T12:15:25Z

.gitignore

@@ -24,6 +24,8 @@ npm-debug.log*
 yarn-debug.log*
 yarn-error.log*

+.jest/
+


This should not be here.

donce · 2018-10-16T12:27:18Z

js/app/app-tequilapi.ts

+  protected proposalFetcher = new ProposalsFetcher(api)
+  protected statusFetcher = new StatusFetcher(api)
+  protected ipFetcher = new IPFetcher(api)
+  protected statsFetcher = new StatsFetcher(api)


donce · 2018-10-16T12:31:11Z

js/app/app-tequilapi.ts

+   * Tries to connect to selected VPN server
+   * @returns {Promise<void>}
+   */
+  public async connect(): Promise<void> {


should be protected.

donce · 2018-10-16T12:34:37Z

js/libraries/logger.ts

+class Logger {
+  private debugMode: boolean = false
+
+  public showDebugMessages(): void {


logObservableChanges ?

donce · 2018-10-16T12:40:20Z

js/app/app.tsx

-import {observer} from "mobx-react/native";
+import Stats from './stats'
+
+logger.showDebugMessages()


this side-effect is a surprise

js/libraries/favorite-storage.ts

donce · 2018-10-16T13:30:07Z

js/libraries/logger.ts

+    if (this.debugMode) {
+      return
+    }
+    this.debugMode = true


So debugMode ensures that this is run only once - I though that it's a flag you have to change to true to get the debug mode, as #define DEBUG 0 in old days :D
Can we rename this to express it's purpose more accuratly?

logingStarted ?

donce · 2018-10-16T13:32:09Z

js/libraries/mysterium-client.ts


 /**
 * This exposes the native MysteriumClient module as a JS module.
 */
-export default class MysteriumClient {
-  _client: any
+export default class MysteriumClient implements IMysteriumClient {


Why do we need this class?

Separate task

js/libraries/unitConverter.ts

tslint.json

js/fetchers/ip-fetcher.ts

donce · 2018-10-16T14:13:15Z

js/fetchers/ip-fetcher.ts


-  constructor (api: TequilapiClient) {
+  constructor(api: IPFetcherProps) {


Lets call it props?

js/libraries/favorite-storage.ts

donce · 2018-10-16T14:37:17Z

js/libraries/logger.ts

+    if (this.debugMode) {
+      return
+    }
+    this.debugMode = true


logingStarted ?

donce · 2018-10-16T14:39:43Z

js/libraries/mysterium-client.ts


 /**
 * This exposes the native MysteriumClient module as a JS module.
 */
-export default class MysteriumClient {
-  _client: any
+export default class MysteriumClient implements IMysteriumClient {


Separate task

js/libraries/favorite-storage.ts

js/libraries/mysterium-client.ts

tslint.json

donce · 2018-10-17T17:15:44Z

tslint.json

+      "arrow-parens": false,
+
+      "jsx-no-lambda": false,
+      "jsx-no-multiline-js": false


?? Do we really need to disable that? Having complex multiline expressions in jsx doesn't seem better to me.

js/store/app-store.ts

js/fetchers/ip-fetcher.ts

donce · 2018-10-17T17:51:07Z

no-unused-variable is deprecated. Since TypeScript 2.9. Please use the built-in compiler checks instead.

?? So why isn't typescript compiler catching these things, why unused stuff gets through?

js/app/app-tequilapi.ts

js/config.ts

iberflow · 2018-10-17T13:22:57Z

js/app/logger.ts

@@ -0,0 +1,40 @@
+import {reaction} from 'mobx'


!! spaces between brackets

js/app/app-tequilapi.ts

js/app/app.tsx

js/app/logger.ts

js/fetchers/stats-fetcher.ts

js/fetchers/status-fetcher.ts

js/libraries/favorite-storage.ts

js/store/app-store.ts

js/fetchers/proposals-fetcher.ts

donce · 2018-10-18T06:09:14Z

tslint.json

+      "interface-over-type-literal": false,
+      "quotemark": [true, "single", "jsx-double"],
+      "no-return-await": true,
+      "curly": false,


?? Why do we need this exception? We're not using it - removing it does not affect anything.

For reference: https://palantir.github.io/tslint/rules/curly/

I personally like "curly": [true, "ignore-same-line"]
which allows for if (!account) return
instead of

if (!account) { return }

tslint.json

shroomist · 2018-10-18T09:49:21Z

tslint.json

+      "interface-over-type-literal": false,
+      "quotemark": [true, "single", "jsx-double"],
+      "no-return-await": true,
+      "curly": false,


I personally like "curly": [true, "ignore-same-line"]
which allows for if (!account) return
instead of

if (!account) { return }

tslint.json

package.json

js/libraries/unitConverter.ts

…-eslint [MVPN-267] Unify curly spacing with tslint-eslint

mipo47 added 3 commits October 8, 2018 15:43

Enable TsLint and refactor code

b290c45

Enable Prettier and reformat code

fd4206f

Don't forget about eslint rules

93b9430

mipo47 requested review from Arrnas, donce, iberflow and shroomist October 8, 2018 13:00

shroomist reviewed Oct 8, 2018

View reviewed changes

VaidotasSm reviewed Oct 8, 2018

View reviewed changes

Throw away eslint and prettier, restyle code

f420d40

donce suggested changes Oct 9, 2018

View reviewed changes

Arrnas reviewed Oct 10, 2018

View reviewed changes

shroomist suggested changes Oct 11, 2018

View reviewed changes

mipo47 added 5 commits October 15, 2018 11:41

PR fixes

23f77e0

PR fixes 2

6728bfc

fix file names

099fd25

change interfaces to types for component input definition

78ce61b

Add return types and TODOs

987af30

shroomist suggested changes Oct 16, 2018

View reviewed changes

final PR fixes

099ddea

shroomist previously approved these changes Oct 16, 2018

View reviewed changes

.jest is not used in this PR

efd8d34

mipo47 dismissed shroomist’s stale review via efd8d34 October 16, 2018 12:23

donce suggested changes Oct 16, 2018

View reviewed changes

donce reviewed Oct 16, 2018

View reviewed changes

tslint.json Outdated Show resolved Hide resolved

donce reviewed Oct 16, 2018

View reviewed changes

mipo47 added 4 commits October 17, 2018 11:52

refactor after Donatas PR review

896ff98

change null to undefined

58f2e91

throw away fetcher cache

4d9d37d

do not export fetcher props

0709f5a

mipo47 self-assigned this Oct 17, 2018

mipo47 added the help wanted Extra attention is needed label Oct 17, 2018

donce suggested changes Oct 17, 2018

View reviewed changes

donce reviewed Oct 17, 2018

View reviewed changes

js/app/app-tequilapi.ts Outdated Show resolved Hide resolved

js/config.ts Show resolved Hide resolved

iberflow suggested changes Oct 18, 2018

View reviewed changes

donce reviewed Oct 18, 2018

View reviewed changes

iberflow previously approved these changes Oct 18, 2018

View reviewed changes

shroomist reviewed Oct 18, 2018

View reviewed changes

donce dismissed iberflow’s stale review via da3955f October 18, 2018 10:36

donce previously approved these changes Oct 18, 2018

View reviewed changes

iberflow previously approved these changes Oct 18, 2018

View reviewed changes

donce and others added 8 commits October 18, 2018 16:47

Add tslint-eslint-rules

6535ba9

Add object-curly-spaces rule, unify spacing in code

db6d492

Merge pull request #14 from mysteriumnetwork/refactor/MVPN-267-tslint…

a2e5f38

…-eslint [MVPN-267] Unify curly spacing with tslint-eslint

Remove empty return jsdocs

7f2f52f

show error for unused import

bb21597

refactor favorite updating

797b722

PR fixes after Ignas review

d7ef949

simplify tslint config

affc3fc

donce dismissed stale reviews from iberflow and themself via affc3fc October 18, 2018 13:48

donce force-pushed the refactor/MVPN-267-enable-tslint branch from 36d744c to affc3fc Compare October 18, 2018 13:48

donce approved these changes Oct 18, 2018

View reviewed changes

iberflow approved these changes Oct 18, 2018

View reviewed changes

mipo47 merged commit 8f31cb6 into master Oct 18, 2018

Waldz unassigned mipo47 Dec 7, 2018

donce deleted the refactor/MVPN-267-enable-tslint branch December 11, 2018 08:06


		constructor (api: TequilapiClient) {
		constructor(api: IPFetcherProps) {

Refactor/mvpn 267 enable tslint #8

Refactor/mvpn 267 enable tslint #8

Uh oh!

Conversation

mipo47 commented Oct 8, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

shroomist Oct 11, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Arrnas left a comment

Choose a reason for hiding this comment

mipo47 commented Oct 8, 2018 •

edited

Loading

shroomist Oct 11, 2018 •

edited

Loading

mipo47 Oct 15, 2018 •

edited

Loading