[Cleanup] Enable noUnusedLocals/Parameters and remove all refs #877

damassi · 2017-12-07T04:15:34Z

One thing I noticed when I first started working in Emission was all of the unused variables and imports. There's a TSLint rule available, but since the check occurs in TypeScript it never gets fired. This PR enables noUnusedLocals and noUnusedParameters in tsconfig.json and sweeps through the code.

While making edits I also found handful of unguarded error code for which I entered console.errors and FIXME: Handle error notes.

Skip New Tests

There are a couple questions which I've noted inline.

damassi · 2017-12-07T04:18:30Z

.vscode/settings.json

- "typescript.format.enable": false
+ "typescript.format.enable": false,
+ "[json]": {
+ "editor.formatOnSave": false


For whatever reason, rules like "require trailing comma" would auto-fix in .json files making it impossible to produce correct JSON.

damassi · 2017-12-07T04:19:15Z

src/lib/AppRegistry.tsx


+// FIXME: This isnt being used


In some of the more ambiguous "WIP?" areas I've commented out and added a note

This can go, the root component decorator is called screenTrack.

damassi · 2017-12-07T04:21:05Z

src/lib/Components/Artist/Articles/Article.tsx

- } | null
- }
-}
+// FIXME: This is being unused in code


There were many instances of there being a interface Props { ... } at the top of the file that had the exact same props as interface RelayProps { ... } at the bottom of the file, though at the top it typically lacked | null. Why is this? In most instances they're nearly identical so why is one not being used?

The RelayProps were being auto generated. I guess someone didn’t realize these existed and manually added them to Props rather than extending.

The RelayProps that add null are more correct, though.

damassi · 2017-12-07T04:32:23Z

tsconfig.json

- "plugins": [{ "name": "typescript-styled-plugin" }],
- "baseUrl": "src"
+ "target": "es6",
+ "types": ["jest", "react", "react-native"]


Just alpha + addition of noUnusedLocals and noUnusedParameters

damassi · 2017-12-07T04:35:41Z

tslint.json

@@ -5,10 +5,11 @@
 "interface-name": [true, "never-prefix"],
 "max-classes-per-file": [false],
 "member-access": [false, "check-accessor", "check-constructor"],
+ "no-console": [true, ["error", ["warn", "error"]]],
+ "no-unused-variable": [true, { "ignore-pattern": "^_" }],


Alpha + added the ability to ignore variables via _, similar to TypeScript compiler. This combined with new TypeScript setting worked well as TSLint seemed to not check anything but parameters once the TypeScript settings were introduced.

(See palantir/tslint#1618 for related info.)

damassi · 2017-12-07T06:20:23Z

src/lib/Components/ArtworkGrids/InfiniteScrollGrid.tsx

@@ -20,7 +18,7 @@ import { isCloseToBottom } from "lib/utils/isCloseToBottom"
 import { ArtistRelayProps } from "./RelayConnections/ArtistForSaleArtworksGrid"
 import { GeneRelayProps } from "./RelayConnections/GeneArtworksGrid"

-export const PageSize = 10
+import { PAGE_SIZE } from "lib/data/constants"


I saw PAGE_SIZE being repeated all over so added this new constants file here (unless there's a better location)

damassi · 2017-12-07T07:13:13Z

src/lib/Components/Consignments/Submission/geminiUploadToS3.ts

- const filename = file.split(/\//g).pop()
+
+ // FIXME: This is being unused in 103; does this uploader work?
+ // const filename = file.split(/\//g).pop()


This is weird, check out https://github.com/artsy/emission/pull/877/files#diff-1e4221a2b438a2ae235d95aff0c14c71L103 (a few lines down from here)

key: geminiKey + "/${filename}",

Is the uploader even working? Should have backticks for interpolation

Yeah, it is weird, but that's actually what it expects ( this is the web version of this code: https://github.com/artsy/gemup/blob/master/gemup.js#L44 )

alloy

Great stuff thus far @damassi, much appreciated! 👏

alloy · 2017-12-07T10:03:59Z

src/lib/AppRegistry.tsx

@@ -38,7 +35,7 @@ const Artist: React.SFC<ArtistProps> = track<ArtistProps>(props => {
 }
 })(props => <ArtistRenderer {...props} render={renderWithLoadProgress(Containers.Artist, props)} />)

-const Inbox: React.SFC<{}> = track<{}>(props => {
+const Inbox: React.SFC<{}> = track<{}>(_props => {


Maybe just () => { ... } ?

alloy · 2017-12-07T10:05:29Z

src/lib/AppRegistry.tsx

+// const Sale: React.SFC<{ saleID: string }> = ({ saleID }) => {
+// const initialProps = { saleID }
+// return <SaleRenderer {...initialProps} render={renderWithLoadProgress(Containers.Sale, initialProps)} />
+// }


I believe the sale view is a WIP that @ashfurrow works on.

After talking with ash confirmed this is on hold for the time being but will leave it around for now

alloy · 2017-12-07T10:06:58Z

src/lib/Components/Artist/Articles/Article.tsx

- } | null
- }
-}
+// FIXME: This is being unused in code


The RelayProps were being auto generated. I guess someone didn’t realize these existed and manually added them to Props rather than extending.

alloy · 2017-12-07T10:07:41Z

src/lib/Components/Artist/Articles/Article.tsx

- } | null
- }
-}
+// FIXME: This is being unused in code


The RelayProps that add null are more correct, though.

alloy · 2017-12-07T10:14:15Z

src/lib/Components/Consignments/Screens/Overview.tsx

- const isPadHorizontal = Dimensions.get("window").height > 700
+
+ // TODO: Reenable when used
+ // const isPadHorizontal = Dimensions.get("window").height > 700


Maybe just remove for now?

alloy · 2017-12-07T10:59:24Z

src/lib/Containers/Conversation.tsx

 // tslint:disable-next-line:no-var-requires
-const chevron: ImageURISource = require("../../../images/horizontal_chevron.png")
+// const chevron: ImageURISource = require("../../../images/horizontal_chevron.png")


Can this simply be removed?

alloy · 2017-12-07T13:08:30Z

tslint.json

@@ -5,10 +5,11 @@
 "interface-name": [true, "never-prefix"],
 "max-classes-per-file": [false],
 "member-access": [false, "check-accessor", "check-constructor"],
+ "no-console": [true, ["error", ["warn", "error"]]],
+ "no-unused-variable": [true, { "ignore-pattern": "^_" }],


damassi · 2017-12-08T00:37:55Z

Ok addressed all feedback and re-added RelayTypes so ready to go. Was impressed by how few ?'s are used throughout interface props code!

alloy · 2017-12-08T10:33:49Z

src/lib/Components/Artist/Articles/index.tsx

@@ -5,11 +5,11 @@ import { createFragmentContainer, graphql } from "react-relay"
 import SerifText from "../../Text/Serif"
 import Article from "./Article"

-interface Props extends ViewProperties {
+interface Props extends RelayProps, ViewProperties {
 articles: any[]


This prop is overriding the actual one from RelayProps.

alloy · 2017-12-08T10:34:57Z

src/lib/Components/Artist/Artworks/index.tsx

- not_for_sale_artworks: any[]
- for_sale_artworks: any[]
- }
+interface Props extends RelayProps, ViewProperties {
 relay: any


I don’t believe this prop is actually used in this module, but if it is then maybe pull in the appropriate typing from react-relay?

alloy · 2017-12-08T10:35:25Z

src/lib/Components/Artist/Shows/VariableSizeShowsList.tsx

@@ -5,7 +5,7 @@ import { LayoutChangeEvent, StyleSheet, View, ViewProperties, ViewStyle } from "

 import Show from "./Show"

-interface Props extends ViewProperties {
+interface Props extends RelayProps, ViewProperties {
 showSize: "medium" | "large"
 shows: any[]


This is overriding the actual typing from RelayProps.

alloy · 2017-12-08T10:38:01Z

src/lib/Components/RelatedArtists/RelatedArtist.tsx

-// }
+interface RelayProps {
+ artist: {
+ __id: string | null


This prop is not being requested by the fragment, so shouldn’t be here.

damassi · 2017-12-08T22:49:31Z

src/lib/Components/Artist/Artworks/index.tsx

 }

 render() {
 const forSaleCount = this.props.artist.counts.for_sale_artworks
- const otherCount = this.props.artist.counts.artworks - forSaleCount
+ const otherCount = (this.props.artist.counts.artworks as number) - (forSaleCount as number)


@alloy - after fixing my typechecker and eliminating some of those top-of-the file Props overrides things got a little stricter and this one was interesting to try to understand...

If you look here and here (this components child) and notice the types, they both say

boolean | number | string | null

Since they're both cast to any of four types the compiler can't make basic inferences, right? And so casting it explicitly above set it through the whole chain?

Yep, you’re code is correct. Because this value could be of a non number type (according to the typing at least), TS will complain that you can’t just subtract. Casting as a subtype, that is, one of the types in this union allows you to do so.

I don’t recall exactly why these typings are a union like that. Is the field a custom GraphQL scalar? If so, iirc those are indeed not very specific :(

alloy · 2017-12-09T09:40:16Z

All looks good to me. Great work! Merge when you think it’s good to go 👍

damassi assigned alloy Dec 7, 2017

damassi commented Dec 7, 2017

View reviewed changes

damassi force-pushed the chris-add-noUnusedLocals-and-fix branch from 2e65561 to 780b468 Compare December 7, 2017 04:27

artsy-peril bot deleted a comment from ArtsyOpenSource Dec 7, 2017

damassi commented Dec 7, 2017

View reviewed changes

damassi force-pushed the chris-add-noUnusedLocals-and-fix branch from 780b468 to d6c64f6 Compare December 7, 2017 05:08

artsy-peril bot deleted a comment from ArtsyOpenSource Dec 7, 2017

damassi force-pushed the chris-add-noUnusedLocals-and-fix branch from bb4210d to 0b1d05b Compare December 7, 2017 05:40

artsy-peril bot deleted a comment from ArtsyOpenSource Dec 7, 2017

damassi commented Dec 7, 2017

View reviewed changes

artsy-peril bot deleted a comment from ArtsyOpenSource Dec 7, 2017

damassi commented Dec 7, 2017

View reviewed changes

alloy reviewed Dec 7, 2017

View reviewed changes

artsy-submodule force-pushed the chris-add-noUnusedLocals-and-fix branch from 170eb16 to 36d3028 Compare December 7, 2017 23:48

artsy-peril bot deleted a comment from ArtsyOpenSource Dec 7, 2017

artsy-submodule force-pushed the chris-add-noUnusedLocals-and-fix branch from 36d3028 to d414e22 Compare December 8, 2017 00:00