Enforce AirBNB style guide rules (streamlit#1748)

* Require AirBNB style guide (without React) * Automatic style fixes (from eslint --fix) * Manual style fixes, and disable often-violated rules * Enable no-underscore-dangle rule * Enable class-methods-use-this (mark methods as static) * Enable no-nested-ternary * Enable radix rule We already pass radix=10 in our codebase e.g. Radio.tsx * Disable eslint on third-party code * Permit this-less class method since MetricsManager loads a global * Enable no-console Migrate existing console calls to use our custom logger * Fix duplicate import * Check import rules in tsx files - Enable "import/extensions" - Break import cycle in Components - Fix App import name * Enable "import/no-extraneous-dependencies" Mostly by ignoring current violations :P * Disable typescript checks for MetricsManager tests
gbaf · Jul 24, 2020 · 8387038 · 8387038
1 parent 7dbb370
commit 8387038
Show file tree

Hide file tree

Showing 99 changed files with 610 additions and 378 deletions.
diff --git a/frontend/.eslintrc b/frontend/.eslintrc
@@ -5,6 +5,7 @@
     "es6": true
   },
   "extends": [
+    "airbnb-typescript/base",
     // Uses the recommended rules from @eslint-plugin-react
     "plugin:react/recommended",
     // Uses the recommended rules from the @typescript-eslint/eslint-plugin
@@ -20,6 +21,7 @@
   // Specifies the ESLint parser
   "parser": "@typescript-eslint/parser",
   "parserOptions": {
+    "project": "./tsconfig.json",
     "ecmaFeatures": {
       "jsx": true // Allows for the parsing of JSX
     },
@@ -70,13 +72,48 @@
     // It's OK to use ts-ignore when we're missing type definitions
     // for a JS library.
     "@typescript-eslint/ban-ts-ignore": "off",
-    "@typescript-eslint/ban-ts-comment": "off"
+    "@typescript-eslint/ban-ts-comment": "off",
+    // Permit for-of loops (https://stackoverflow.com/a/42237667)
+    "no-restricted-syntax": [
+      "error",
+      "ForInStatement",
+      "LabeledStatement",
+      "WithStatement"
+    ],
+    // Allow foo.hasOwnProperty("bar")
+    "no-prototype-builtins": "off",
+    // Imports should be `import "./FooModule"`, not `import "./FooModule.js"`
+    // We need to configure this to check our .tsx files, see:
+    // https://github.com/benmosher/eslint-plugin-import/issues/1615#issuecomment-577500405
+    "import/extensions": [
+      "error",
+      "ignorePackages",
+      {
+        "js": "never",
+        "jsx": "never",
+        "ts": "never",
+        "tsx": "never"
+      }
+    ],
+    // Disable a bunch of AirBNB rules we're currently in violation of.
+    // TODO: For each one, either fix and reenable, or provide a justification.
+    "import/prefer-default-export": "off",
+    "max-classes-per-file": "off",
+    "no-shadow": "off",
+    "no-param-reassign": "off",
+    "no-plusplus": "off"
   },
   "settings": {
     "react": {
       // Tells eslint-plugin-react to automatically detect
       // the version of React to use
       "version": "detect"
+    },
+    // Check for import violation in all JS-like files
+    "import/resolver": {
+      "node": {
+        "extensions": [".js", ".jsx", ".ts", ".tsx", ".json"]
+      }
     }
   }
 }
diff --git a/frontend/package.json b/frontend/package.json
@@ -125,15 +125,17 @@
     "@types/styletron-react": "^5.0.2",
     "@types/styletron-standard": "^2.0.0",
     "@types/xxhashjs": "^0.2.1",
-    "@typescript-eslint/eslint-plugin": "^3.6.0",
+    "@typescript-eslint/eslint-plugin": "^3.6.1",
     "@typescript-eslint/parser": "^3.6.0",
     "axios-mock-adapter": "^1.17.0",
     "cypress": "4.1.0",
     "cypress-file-upload": "^3.5.1",
     "cypress-image-snapshot": "^3.1.1",
     "enzyme": "^3.10.0",
     "enzyme-adapter-react-16": "^1.15.2",
+    "eslint-config-airbnb-typescript": "^9.0.0",
     "eslint-config-prettier": "^6.2.0",
+    "eslint-plugin-import": "^2.22.0",
     "eslint-plugin-prettier": "^3.1.3",
     "eslint-plugin-react": "^7.14.3",
     "fetch-mock": "^7.3.9",

diff --git a/frontend/src/App.test.tsx b/frontend/src/App.test.tsx
@@ -72,7 +72,7 @@ describe("App", () => {
   })
 
   afterEach(() => {
-    SessionInfo["singleton"] = undefined
+    SessionInfo.singleton = undefined
   })
 
   it("renders without crashing", () => {

diff --git a/frontend/src/App.tsx b/frontend/src/App.tsx
@@ -41,7 +41,13 @@ import {
   ReportElement,
   SimpleElement,
 } from "lib/DeltaParser"
-import { setCookie } from "lib/utils"
+import {
+  setCookie,
+  flattenElements,
+  hashString,
+  isEmbeddedInIFrame,
+  makeElementWithInfoText,
+} from "lib/utils"
 import {
   BackMsg,
   Delta,
@@ -58,12 +64,7 @@ import { RERUN_PROMPT_MODAL_DIALOG } from "lib/baseconsts"
 import { SessionInfo } from "lib/SessionInfo"
 import { MetricsManager } from "lib/MetricsManager"
 import { FileUploadClient } from "lib/FileUploadClient"
-import {
-  flattenElements,
-  hashString,
-  isEmbeddedInIFrame,
-  makeElementWithInfoText,
-} from "lib/utils"
+
 import { logError, logMessage } from "lib/log"
 // WARNING: order matters
 import "assets/css/theme.scss"
@@ -102,12 +103,19 @@ declare global {
 
 export class App extends PureComponent<Props, State> {
   private readonly sessionEventDispatcher: SessionEventDispatcher
+
   private readonly statusWidgetRef: React.RefObject<StatusWidget>
+
   private connectionManager: ConnectionManager | null
+
   private readonly widgetMgr: WidgetStateManager
+
   private readonly uploadClient: FileUploadClient
+
   private elementListBuffer: Elements | null
+
   private elementListBufferTimerIsSet: boolean
+
   private readonly componentRegistry: ComponentRegistry
 
   constructor(props: Props) {
@@ -191,7 +199,7 @@ export class App extends PureComponent<Props, State> {
     logError(errorNode)
     const newDialog: DialogProps = {
       type: DialogType.WARNING,
-      title: title,
+      title,
       msg: errorNode,
       onClose: () => {},
     }
@@ -201,7 +209,7 @@ export class App extends PureComponent<Props, State> {
   /**
    * Checks if the code version from the backend is different than the frontend
    */
-  hasStreamlitVersionChanged(initializeMsg: Initialize): boolean {
+  static hasStreamlitVersionChanged(initializeMsg: Initialize): boolean {
     if (SessionInfo.isSet()) {
       const { streamlitVersion: currentStreamlitVersion } = SessionInfo.current
       const { environmentInfo } = initializeMsg
@@ -248,9 +256,8 @@ export class App extends PureComponent<Props, State> {
       const whichOne = obj[name]
       if (whichOne in funcs) {
         return funcs[whichOne](obj[whichOne])
-      } else {
-        throw new Error(`Cannot handle ${name} "${whichOne}".`)
       }
+      throw new Error(`Cannot handle ${name} "${whichOne}".`)
     }
 
     try {
@@ -280,7 +287,7 @@ export class App extends PureComponent<Props, State> {
   handleUploadReportProgress = (progress: string | number): void => {
     const newDialog: DialogProps = {
       type: DialogType.UPLOAD_PROGRESS,
-      progress: progress,
+      progress,
       onClose: () => {},
     }
     this.openDialog(newDialog)
@@ -289,7 +296,7 @@ export class App extends PureComponent<Props, State> {
   handleReportUploaded = (url: string): void => {
     const newDialog: DialogProps = {
       type: DialogType.UPLOADED,
-      url: url,
+      url,
       onClose: () => {},
     }
     this.openDialog(newDialog)
@@ -319,14 +326,14 @@ export class App extends PureComponent<Props, State> {
       throw new Error("InitializeMsg is missing a required field")
     }
 
-    if (this.hasStreamlitVersionChanged(initializeMsg)) {
+    if (App.hasStreamlitVersionChanged(initializeMsg)) {
       window.location.reload()
 
       return
     }
 
     SessionInfo.current = new SessionInfo({
-      sessionId: sessionId,
+      sessionId,
       streamlitVersion: environmentInfo.streamlitVersion,
       pythonVersion: environmentInfo.pythonVersion,
       installationId: userInfo.installationId,
@@ -358,8 +365,8 @@ export class App extends PureComponent<Props, State> {
   handleSessionStateChanged = (stateChangeProto: ISessionState): void => {
     this.setState((prevState: State) => {
       // Determine our new ReportRunState
-      let reportRunState = prevState.reportRunState
-      let dialog = prevState.dialog
+      let { reportRunState } = prevState
+      let { dialog } = prevState
 
       if (
         stateChangeProto.reportIsRunning &&
@@ -491,14 +498,14 @@ export class App extends PureComponent<Props, State> {
       // This step removes from the WidgetManager the state of those widgets
       // that are not shown on the page.
       if (this.elementListBuffer) {
-        const active_widget_ids = flattenElements(this.elementListBuffer.main)
+        const activeWidgetIds = flattenElements(this.elementListBuffer.main)
           .union(flattenElements(this.elementListBuffer.sidebar))
           .map((e: SimpleElement) => {
             const type = e.get("type")
             return e.get(type).get("id") as string
           })
           .filter(id => id != null)
-        this.widgetMgr.clean(active_widget_ids)
+        this.widgetMgr.clean(activeWidgetIds)
       }
 
       // Tell the ConnectionManager to increment the message cache run
@@ -589,7 +596,7 @@ export class App extends PureComponent<Props, State> {
    */
   saveSettings = (newSettings: UserSettings): void => {
     const prevRunOnSave = this.state.userSettings.runOnSave
-    const runOnSave = newSettings.runOnSave
+    const { runOnSave } = newSettings
 
     this.setState({ userSettings: newSettings })
 
@@ -639,7 +646,7 @@ export class App extends PureComponent<Props, State> {
             const elements: Elements = {
               ...this.elementListBuffer,
             }
-            this.setState({ elements: elements })
+            this.setState({ elements })
           }
         }
       }, ELEMENT_LIST_BUFFER_TIMEOUT_MS)

diff --git a/frontend/src/components/core/Block/Block.tsx b/frontend/src/components/core/Block/Block.tsx
@@ -109,13 +109,8 @@ class Block extends PureComponent<Props> {
 
         if (element instanceof List) {
           return this.renderBlock(element as BlockElement, index, width)
-        } else {
-          return this.renderElementWithErrorBoundary(
-            reportElement,
-            index,
-            width
-          )
         }
+        return this.renderElementWithErrorBoundary(reportElement, index, width)
       })
       .filter((node: ReactNode | null): ReactNode => node != null)
   }
@@ -125,11 +120,11 @@ class Block extends PureComponent<Props> {
       // If a rerun was just requested, all of our current elements
       // are about to become stale.
       return true
-    } else if (this.props.reportRunState === ReportRunState.RUNNING) {
+    }
+    if (this.props.reportRunState === ReportRunState.RUNNING) {
       return reportElement.get("reportId") !== this.props.reportId
-    } else {
-      return false
     }
+    return false
   }
 
   private renderBlock(

diff --git a/frontend/src/components/core/Countdown/Countdown.tsx b/frontend/src/components/core/Countdown/Countdown.tsx
@@ -60,7 +60,7 @@ class Countdown extends PureComponent<Props, State> {
       <div
         className="countdown"
         onAnimationEnd={this.onAnimationEnd}
-        key={"frame" + countdown}
+        key={`frame${countdown}`}
       >
         {/* The key forces DOM mutations, for animation to restart. */}
         <span>{countdown}</span>

diff --git a/frontend/src/components/core/Maybe/Maybe.tsx b/frontend/src/components/core/Maybe/Maybe.tsx
@@ -24,6 +24,7 @@ export interface Props {
 export interface State {}
 
 class Maybe extends React.Component<Props, State> {
+  // eslint-disable-next-line class-methods-use-this
   public shouldComponentUpdate(
     nextProps: Readonly<Props>,
     nextState: Readonly<State>,

diff --git a/frontend/src/components/core/StatusWidget/StatusWidget.tsx b/frontend/src/components/core/StatusWidget/StatusWidget.tsx
@@ -108,9 +108,11 @@ const PROMPT_DISPLAY_HOVER_TIMEOUT_MS = 1.0 * 1000
 export class StatusWidget extends PureComponent<Props, State> {
   /** onSessionEvent signal connection */
   private sessionEventConn?: SignalConnection
+
   private curView?: ReactNode
 
   private readonly minimizePromptTimer = new Timer()
+
   private readonly keyHandlers: {
     [key: string]: (keyEvent?: KeyboardEvent) => void
   }
@@ -243,10 +245,8 @@ export class StatusWidget extends PureComponent<Props, State> {
         // re-running the report in a second or two, but we can appear
         // more responsive by claiming it's started immemdiately.
         return this.renderReportIsRunning()
-      } else if (
-        !RERUN_PROMPT_MODAL_DIALOG &&
-        this.state.reportChangedOnDisk
-      ) {
+      }
+      if (!RERUN_PROMPT_MODAL_DIALOG && this.state.reportChangedOnDisk) {
         return this.renderRerunReportPrompt()
       }
     }

diff --git a/frontend/src/components/core/StreamlitDialog/SettingsDialog.tsx b/frontend/src/components/core/StreamlitDialog/SettingsDialog.tsx
@@ -15,8 +15,7 @@
  * limitations under the License.
  */
 
-import * as React from "react"
-import { ChangeEvent, PureComponent, ReactNode } from "react"
+import React, { ChangeEvent, PureComponent, ReactNode } from "react"
 import { Button, Modal, ModalBody, ModalFooter, ModalHeader } from "reactstrap"
 import { UserSettings } from "./UserSettings"
 
@@ -31,7 +30,7 @@ export interface Props {
  * Implements a dialog that is used to configure user settings.
  */
 export class SettingsDialog extends PureComponent<Props, UserSettings> {
-  private _settings: UserSettings
+  private activeSettings: UserSettings
 
   constructor(props: Props) {
     super(props)
@@ -40,7 +39,7 @@ export class SettingsDialog extends PureComponent<Props, UserSettings> {
     this.state = { ...this.props.settings }
 
     // Holds the actual settings that Streamlit is using.
-    this._settings = { ...this.props.settings }
+    this.activeSettings = { ...this.props.settings }
   }
 
   public render = (): ReactNode => {
@@ -94,7 +93,7 @@ export class SettingsDialog extends PureComponent<Props, UserSettings> {
   }
 
   private handleDialogOpen = (): void => {
-    this.setState({ ...this._settings })
+    this.setState({ ...this.activeSettings })
   }
 
   private changeSingleSetting = (name: string, value: boolean): void => {
@@ -115,8 +114,8 @@ export class SettingsDialog extends PureComponent<Props, UserSettings> {
   }
 
   private handleSaveButtonClick = (): void => {
-    this._settings = { ...this.state }
-    this.props.onSave(this._settings)
+    this.activeSettings = { ...this.state }
+    this.props.onSave(this.activeSettings)
     this.props.onClose()
   }
 }
diff --git a/frontend/src/components/core/StreamlitDialog/StreamlitDialog.tsx b/frontend/src/components/core/StreamlitDialog/StreamlitDialog.tsx
@@ -32,8 +32,8 @@ import {
   Props as ScriptChangedDialogProps,
 } from "components/core/StreamlitDialog/ScriptChangedDialog"
 import { IException } from "autogen/proto"
-import { Props as SettingsDialogProps, SettingsDialog } from "./SettingsDialog"
 import { SessionInfo } from "lib/SessionInfo"
+import { Props as SettingsDialogProps, SettingsDialog } from "./SettingsDialog"
 
 import "./StreamlitDialog.scss"