migrate Firestore to eslint (#1859)

* migrate Firestore to eslint

* [AUTOMATED]: Prettier Code Styling

* replace tslint suppresion with eslint ones

* disable no used var for params in firestore

* [AUTOMATED]: Prettier Code Styling

* enable no-console for test files

* disable no-floating-promises for test files

* 1. enable no-duplicate-imports 2. update deps

* address mike's comments

* [AUTOMATED]: Prettier Code Styling

* move comments to the same line as the suppression

* [AUTOMATED]: Prettier Code Styling

* remove unneeded comment

* revert local changes

* enable no-explicit-any in test files

* fix lint issues after rebasing with master

* [AUTOMATED]: Prettier Code Styling

* move protobufjs to devdeps

* [AUTOMATED]: Prettier Code Styling

* remove leftover suppression

Author: Feiyang1

packages/firestore/.eslintrc.json

@@ -2,5 +2,30 @@
     "extends": "../../config/.eslintrc.json",
     "parserOptions": {
         "project": "tsconfig.json"
-    }
+    },
+    "plugins": [
+        "import"
+    ],
+    "rules": {
+        "no-console":[ "error", { "allow": ["warn", "error"] }],
+        "import/no-default-export": "error",
+        "@typescript-eslint/no-unused-vars": [
+            "error",
+            {
+                "varsIgnorePattern": "^_",
+                "args": "none"
+            }
+        ]
+    },
+    "overrides": [
+        {
+            "files": [
+                "**/*.test.ts",
+                "**/test/**/*.ts"
+            ],
+            "rules": {
+                "@typescript-eslint/no-explicit-any": "error"
+            }
+        }
+    ]
 }

packages/firestore/index.node.ts

@@ -18,7 +18,6 @@
 import firebase from '@firebase/app';
 import { FirebaseNamespace } from '@firebase/app-types';
 import * as types from '@firebase/firestore-types';
-import { Firestore } from './src/api/database';
 import { configureForFirebase } from './src/platform/config';
 import './src/platform_node/node_init';
 

packages/firestore/package.json

@@ -7,9 +7,8 @@
     "build": "rollup -c",
     "build:console": "node tools/console.build.js",
     "dev": "rollup -c -w",
-    "lint": "tslint -p tsconfig.json -c tslint.json 'src/**/*.ts' 'test/**/*.ts'",
-    "lint:fix": "tslint --fix -p tsconfig.json -c tslint.json 'src/**/*.ts' 'test/**/*.ts'",
-    "lint:eslint": "eslint -c .eslintrc.json '**/*.ts' --ignore-path '../../.gitignore'",
+    "lint": "eslint -c .eslintrc.json '**/*.ts' --ignore-path '../../.gitignore'",
+    "lint:fix": "eslint --fix -c .eslintrc.json '**/*.ts' --ignore-path '../../.gitignore'",
     "prettier": "prettier --write 'src/**/*.js' 'test/**/*.js' 'src/**/*.ts' 'test/**/*.ts'",
     "test": "run-s lint test:all",
     "test:all": "run-p test:browser test:emulator",
@@ -33,6 +32,7 @@
     "@firebase/logger": "0.1.17",
     "@firebase/webchannel-wrapper": "0.2.21",
     "@grpc/proto-loader": "^0.5.0",
+    "@firebase/util": "0.2.20",
     "grpc": "1.20.3",
     "tslib": "1.9.3"
   },
@@ -82,7 +82,8 @@
     "eslint": "5.16.0",
     "@typescript-eslint/parser": "1.10.2",
     "@typescript-eslint/eslint-plugin": "1.10.2",
-    "@typescript-eslint/eslint-plugin-tslint": "1.10.2"
+    "@typescript-eslint/eslint-plugin-tslint": "1.10.2",
+    "eslint-plugin-import": "2.17.3"
   },
   "repository": {
     "directory": "packages/firestore",
@@ -99,4 +100,4 @@
     ],
     "reportDir": "./coverage/node"
   }
-}
+}

packages/firestore/src/api/blob.ts

@@ -136,8 +136,7 @@ export class Blob {
 // instanceof checks.
 // For our internal TypeScript code PublicBlob doesn't exist as a type, and so
 // we need to use Blob as type and export it too.
-// tslint:disable-next-line:variable-name We're treating this as a class name.
-export let PublicBlob = makeConstructorPrivate(
+export const PublicBlob = makeConstructorPrivate(
   Blob,
   'Use Blob.fromUint8Array() or Blob.fromBase64String() instead.'
 );

packages/firestore/src/api/credentials.ts

@@ -104,8 +104,6 @@ export class EmptyCredentialsProvider implements CredentialsProvider {
    */
   private changeListener: CredentialChangeListener | null = null;
 
-  constructor() {}
-
   getToken(): Promise<Token | null> {
     return Promise.resolve<Token | null>(null);
   }

packages/firestore/src/api/database.ts

@@ -74,7 +74,9 @@ import {
   validateStringEnum,
   valueDescription
 } from '../util/input_validation';
+// eslint-disable-next-line import/no-duplicates
 import * as log from '../util/log';
+// eslint-disable-next-line import/no-duplicates
 import { LogLevel } from '../util/log';
 import { AutoId } from '../util/misc';
 import * as objUtils from '../util/obj';
@@ -103,11 +105,6 @@ import {
   UserDataConverter
 } from './user_data_converter';
 
-// The objects that are a part of this API are exposed to third-parties as
-// compiled javascript so we want to flag our private members with a leading
-// underscore to discourage their use.
-// tslint:disable:strip-private-property-underscore
-
 // settings() defaults:
 const DEFAULT_HOST = 'firestore.googleapis.com';
 const DEFAULT_SSL = true;
@@ -158,7 +155,7 @@ class FirestoreSettings {
   readonly forceLongPolling: boolean;
 
   // Can be a google-auth-library or gapi client.
-  // tslint:disable-next-line:no-any
+  // eslint-disable-next-line @typescript-eslint/no-explicit-any
   credentials?: any;
 
   constructor(settings: PrivateSettings) {
@@ -298,6 +295,9 @@ class FirestoreConfig {
  * The root reference to the database.
  */
 export class Firestore implements firestore.FirebaseFirestore, FirebaseService {
+  // The objects that are a part of this API are exposed to third-parties as
+  // compiled javascript so we want to flag our private members with a leading
+  // underscore to discourage their use.
   private readonly _config: FirestoreConfig;
   readonly _databaseId: DatabaseId;
 
@@ -480,7 +480,7 @@ export class Firestore implements firestore.FirebaseFirestore, FirebaseService {
 
     const databaseInfo = this.makeDatabaseInfo();
 
-    const preConverter = (value: unknown) => {
+    const preConverter = (value: unknown): unknown => {
       if (value instanceof DocumentReference) {
         const thisDb = this._config.databaseId;
         const otherDb = value.firestore._config.databaseId;
@@ -1116,7 +1116,7 @@ export class DocumentReference implements firestore.DocumentReference {
     options: ListenOptions,
     observer: PartialObserver<firestore.DocumentSnapshot>
   ): Unsubscribe {
-    let errHandler = (err: Error) => {
+    let errHandler = (err: Error): void => {
       console.error('Uncaught Error in onSnapshot:', err);
     };
     if (observer.error) {
@@ -1387,16 +1387,6 @@ export class DocumentSnapshot implements firestore.DocumentSnapshot {
 
 export class QueryDocumentSnapshot extends DocumentSnapshot
   implements firestore.QueryDocumentSnapshot {
-  constructor(
-    firestore: Firestore,
-    key: DocumentKey,
-    document: Document,
-    fromCache: boolean,
-    hasPendingWrites: boolean
-  ) {
-    super(firestore, key, document, fromCache, hasPendingWrites);
-  }
-
   data(options?: SnapshotOptions): firestore.DocumentData {
     const data = super.data(options);
     assert(
@@ -1808,7 +1798,7 @@ export class Query implements firestore.Query {
     options: ListenOptions,
     observer: PartialObserver<firestore.QuerySnapshot>
   ): Unsubscribe {
-    let errHandler = (err: Error) => {
+    let errHandler = (err: Error): void => {
       console.error('Uncaught Error in onSnapshot:', err);
     };
     if (observer.error) {
@@ -1830,7 +1820,7 @@ export class Query implements firestore.Query {
       asyncObserver,
       options
     );
-    return () => {
+    return (): void => {
       asyncObserver.mute();
       firestoreClient.unlisten(internalListener);
     };
@@ -2457,7 +2447,6 @@ function resultChangeType(type: ChangeType): firestore.DocumentChangeType {
 
 // We're treating the variables as class names, so disable checking for lower
 // case variable names.
-// tslint:disable:variable-name
 export const PublicFirestore = makeConstructorPrivate(
   Firestore,
   'Use firebase.firestore() instead.'
@@ -2484,4 +2473,3 @@ export const PublicCollectionReference = makeConstructorPrivate(
   CollectionReference,
   'Use firebase.firestore().collection() instead.'
 );
-// tslint:enable:variable-name

packages/firestore/src/api/field_path.ts

@@ -28,7 +28,6 @@ import {
 // The objects that are a part of this API are exposed to third-parties as
 // compiled javascript so we want to flag our private members with a leading
 // underscore to discourage their use.
-// tslint:disable:strip-private-property-underscore
 
 /**
  * A FieldPath refers to a field in a document. The path may consist of a single

packages/firestore/src/api/field_value.ts

@@ -29,7 +29,6 @@ import {
  * An opaque base class for FieldValue sentinel objects in our public API,
  * with public static methods for creating said sentinel objects.
  */
-// tslint:disable-next-line:class-as-namespace  We use this as a base class.
 export abstract class FieldValueImpl implements firestore.FieldValue {
   protected constructor(readonly _methodName: string) {}
 
@@ -109,7 +108,6 @@ export class NumericIncrementFieldValueImpl extends FieldValueImpl {
 // PublicFieldValue can be used interchangeably in instanceof checks.
 // For our internal TypeScript code PublicFieldValue doesn't exist as a type,
 // and so we need to use FieldValueImpl as type and export it too.
-// tslint:disable-next-line:variable-name  We treat this as a class name.
 export const PublicFieldValue = makeConstructorPrivate(
   FieldValueImpl,
   'Use FieldValue.<field>() instead.'

packages/firestore/src/api/user_data_converter.ts

@@ -20,8 +20,10 @@ import * as firestore from '@firebase/firestore-types';
 import { Timestamp } from '../api/timestamp';
 import { DatabaseId } from '../core/database_info';
 import { DocumentKey } from '../model/document_key';
-import { FieldValue, NumberValue, ObjectValue } from '../model/field_value';
 import {
+  FieldValue,
+  NumberValue,
+  ObjectValue,
   ArrayValue,
   BlobValue,
   BooleanValue,
@@ -33,6 +35,7 @@ import {
   StringValue,
   TimestampValue
 } from '../model/field_value';
+
 import {
   FieldMask,
   FieldTransform,
@@ -47,8 +50,7 @@ import { assert, fail } from '../util/assert';
 import { Code, FirestoreError } from '../util/error';
 import { isPlainObject, valueDescription } from '../util/input_validation';
 import { primitiveComparator } from '../util/misc';
-import * as objUtils from '../util/obj';
-import { Dict } from '../util/obj';
+import { Dict, forEach, isEmpty } from '../util/obj';
 import { SortedMap } from '../util/sorted_map';
 import * as typeUtils from '../util/types';
 
@@ -394,7 +396,7 @@ export class UserDataConverter {
 
     let fieldMaskPaths = new SortedSet<FieldPath>(FieldPath.comparator);
     let updateData = ObjectValue.EMPTY;
-    objUtils.forEach(input as Dict<unknown>, (key, value) => {
+    forEach(input as Dict<unknown>, (key, value) => {
       const path = fieldPathFromDotSeparatedString(methodName, key);
 
       const childContext = context.childContextForFieldPath(path);
@@ -544,14 +546,14 @@ export class UserDataConverter {
   private parseObject(obj: Dict<unknown>, context: ParseContext): FieldValue {
     let result = new SortedMap<string, FieldValue>(primitiveComparator);
 
-    if (objUtils.isEmpty(obj)) {
+    if (isEmpty(obj)) {
       // If we encounter an empty object, we explicitly add it to the update
       // mask to ensure that the server creates a map entry.
       if (context.path && context.path.length > 0) {
         context.fieldMask.push(context.path);
       }
     } else {
-      objUtils.forEach(obj, (key: string, val: unknown) => {
+      forEach(obj, (key: string, val: unknown) => {
         const parsedValue = this.parseData(
           val,
           context.childContextForField(key)

packages/firestore/src/core/event_manager.ts

@@ -21,8 +21,7 @@ import { ObjectMap } from '../util/obj_map';
 import { Query } from './query';
 import { SyncEngine, SyncEngineListener } from './sync_engine';
 import { OnlineState, TargetId } from './types';
-import { DocumentViewChange } from './view_snapshot';
-import { ChangeType, ViewSnapshot } from './view_snapshot';
+import { DocumentViewChange, ChangeType, ViewSnapshot } from './view_snapshot';
 
 /**
  * Holds the listeners and the last received ViewSnapshot for a query being
@@ -72,7 +71,9 @@ export class EventManager implements SyncEngineListener {
 
     listener.applyOnlineStateChange(this.onlineState);
 
-    if (queryInfo.viewSnap) listener.onViewSnapshot(queryInfo.viewSnap);
+    if (queryInfo.viewSnap) {
+      listener.onViewSnapshot(queryInfo.viewSnap);
+    }
 
     if (firstListen) {
       return this.syncEngine.listen(query).then(targetId => {

packages/firestore/src/core/firestore_client.ts

@@ -252,7 +252,6 @@ export class FirestoreClient {
           if (!this.canFallback(error)) {
             throw error;
           }
-
           console.warn(
             'Error enabling offline persistence. Falling back to' +
               ' persistence disabled: ' +
@@ -431,14 +430,14 @@ export class FirestoreClient {
 
         const remoteStoreOnlineStateChangedHandler = (
           onlineState: OnlineState
-        ) =>
+        ): void =>
           this.syncEngine.applyOnlineStateChange(
             onlineState,
             OnlineStateSource.RemoteStore
           );
         const sharedClientStateOnlineStateChangedHandler = (
           onlineState: OnlineState
-        ) =>
+        ): void =>
           this.syncEngine.applyOnlineStateChange(
             onlineState,
             OnlineStateSource.SharedClientState

packages/firestore/src/core/query.ts

@@ -310,7 +310,9 @@ export class Query {
     let comparedOnKeyField = false;
     for (const orderBy of this.orderBy) {
       const comp = orderBy.compare(d1, d2);
-      if (comp !== 0) return comp;
+      if (comp !== 0) {
+        return comp;
+      }
       comparedOnKeyField = comparedOnKeyField || orderBy.field.isKeyField();
     }
     // Assert that we actually compared by key
@@ -632,10 +634,6 @@ export class FieldFilter extends Filter {
 
 /** Filter that matches on key fields (i.e. '__name__'). */
 export class KeyFieldFilter extends FieldFilter {
-  constructor(field: FieldPath, op: Operator, value: RefValue) {
-    super(field, op, value);
-  }
-
   matches(doc: Document): boolean {
     const refValue = this.value as RefValue;
     const comparison = DocumentKey.comparator(doc.key, refValue.key);

packages/firestore/src/core/sync_engine.ts

@@ -248,7 +248,6 @@ export class SyncEngine implements RemoteSyncer, SharedClientStateSyncer {
         .then(remoteKeys => {
           const view = new View(query, remoteKeys);
           const viewDocChanges = view.computeDocChanges(docs);
-          // tslint:disable-next-line:max-line-length Prettier formats this exceed 100 characters.
           const synthesizedTargetChange = TargetChange.createSynthesizedTargetChangeForCurrentChange(
             queryData.targetId,
             current && this.onlineState !== OnlineState.Offline
@@ -388,7 +387,7 @@ export class SyncEngine implements RemoteSyncer, SharedClientStateSyncer {
   ): Promise<T> {
     assert(retries >= 0, 'Got negative number of retries for transaction.');
     const transaction = this.remoteStore.createTransaction();
-    const wrappedUpdateFunction = () => {
+    const wrappedUpdateFunction = (): Promise<T> => {
       try {
         const userPromise = updateFunction(transaction);
         if (
@@ -976,8 +975,6 @@ export class SyncEngine implements RemoteSyncer, SharedClientStateSyncer {
         case 'not-current': {
           return this.localStore.getNewDocumentChanges().then(
             async changes => {
-              // tslint and prettier disagree about their preferred line length.
-              // tslint:disable-next-line:max-line-length
               const synthesizedRemoteEvent = RemoteEvent.createSynthesizedRemoteEventForCurrentChange(
                 targetId,
                 state === 'current'

packages/firestore/src/core/transaction.ts

@@ -17,8 +17,8 @@
 
 import { ParsedSetData, ParsedUpdateData } from '../api/user_data_converter';
 import { documentVersionMap } from '../model/collections';
-import { Document, NoDocument } from '../model/document';
-import { MaybeDocument } from '../model/document';
+import { Document, NoDocument, MaybeDocument } from '../model/document';
+
 import { DocumentKey } from '../model/document_key';
 import { DeleteMutation, Mutation, Precondition } from '../model/mutation';
 import { Datastore } from '../remote/datastore';

packages/firestore/src/core/view.ts

@@ -468,7 +468,7 @@ export class View {
 }
 
 function compareChangeType(c1: ChangeType, c2: ChangeType): number {
-  const order = (change: ChangeType) => {
+  const order = (change: ChangeType): 0 | 1 | 2 => {
     switch (change) {
       case ChangeType.Added:
         return 1;