migrate Firestore to eslint #1859

Feiyang1 · 2019-06-08T00:48:17Z

No description provided.

schmidt-sebastian · 2019-06-08T03:29:50Z

packages/firestore/src/api/database.ts

@@ -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 => {


Is there a way to turn off the return-type enforcement for very simple functions?

The rule doesn't provide an option to do that currently.
An alternative is

type ErrorHandler = (err: Error) => void; let errHandler: ErrorHandler = (err) => { ... }

I vote option #1 then (leave as is in the current PR).

The TSLint rule had an option to ignore arrow functions, which I was using in Messaging and Installations. If this rule has a similar option, I think it's safe to enable that.

schmidt-sebastian · 2019-06-08T03:33:10Z

packages/firestore/src/local/memory_mutation_queue.ts

  ): PersistencePromise<ProtoByteString> {
    return PersistencePromise.resolve(this.lastStreamToken);
  }

  setLastStreamToken(
-    transaction: PersistenceTransaction,
+    _transaction: PersistenceTransaction,


Can we enable after-used? (see https://eslint.org/docs/rules/no-unused-vars#args). That would also make the Storage PR smaller, and the code prettier :)

I'd like to have _ prefixed to all unused parameters regardless of the position, because it's a nice visual cue to show it is unused, which improves the code readability.
@mikelehen Would you like to help us break the tie here? :)

I'm not in favor of after-used since I do think it would end up with our code being inconsistent in a weird way (some unused parameters have _ and others don't, depending on the order). I want the code to be consistent.

That said, I'm on the fence of which way I want to be consistent (having underscores or no underscores for unused parameters). To be honest, scanning through the diff, I'm not sure how much it's adding. The vast majority of the violations are cases where we're just implementing a contract (an interface method or callback function) which leads to intentional "unused" parameters.

The _ makes it more visible that they're unused but VS Code actually colors unused variables differently already, so I'm not sure how much value the _ adds (and it becomes something we have to manually do / remember to undo if we use the variable).

I did notice that we had one case where the unused parameter caught by eslint was actually indicative of a coding mistake, though you missed it in your fixing. 😛 #1863

@schmidt-sebastian and @wilhuff have both gently pushed back on this (due to the noise and overloading how we use _ too much) and mostly the lint error seems to be triggering false-positives, and so I'm worried it may just be adding unnecessary visual noise to our code.

So at this point, I think I'm actually leaning towards disabling no-unused-vars for parameters, at least in Firestore... Sorry. WDYT @Feiyang1

I think there is value for _ even for intentional "unused" variables, that is _ makes the intention explicit.
_ is also helpful during code review since code review tools won't color unused variables differently like VSCode. Similarly It will work regardless of the IDE.
I agree it will add some cognitive and maintenance burden, and it sounds like Firestore use _ for other purpose as well, so I'm happy to disable this rule for Firestore if you think that works better for Firestore. Let me know :)

Yeah, I think we want to disable it for Firestore. Thanks / sorry!

Done. disabled no-unused-vars for function params, but it still checks variables.

Feiyang1 · 2019-06-14T00:15:04Z

@mikelehen gentle ping :)

mikelehen

Thanks for tackling this and sorry for the super slow review... This mostly looks good, but I'm not sure why you disabled some of the lint checks in tests... I think we'll want to re-enable those and then revert a bunch of your suppression removals.

packages/firestore/src/api/database.ts

packages/firestore/src/api/field_value.ts

packages/firestore/src/core/sync_engine.ts

packages/firestore/src/local/persistence_promise.ts

mikelehen · 2019-06-18T23:24:01Z

packages/firestore/src/util/error.ts

@@ -24,7 +24,7 @@ import * as firestore from '@firebase/firestore-types';
 export type Code = firestore.FirestoreErrorCode;

 // TODO(mcg): Change to a string enum once we've upgraded to typescript 2.4.
-//  tslint:disable-next-line:variable-name Intended to look like a TS 2.4 enum
+// Intended to look like a TS 2.4 enum


comment not necessary now.

packages/firestore/test/integration/bootstrap.ts

packages/firestore/test/unit/local/local_store.test.ts

packages/firestore/test/unit/remote/remote_event.test.ts

mikelehen · 2019-06-18T23:44:59Z

packages/firestore/.eslintrc.json

+            "**/test/**/*.ts"
+        ],
+        "rules": {
+            "no-console": "off"


why did you disable this? People often add console.log() statements while debugging tests and then accidentally forget to remove them.

There is a few places where it seems we use console.log intentionally, so I decided to just allow it.
I will enable "no-console" for test related files and add suppression instead. If anything of them should actually be removed, let me know, then I can remove them.

Feiyang1

In addition to address comments, I also fixed issues flagged by import/no-duplicate which was enabled after the PR was opened.

Feiyang1 · 2019-06-20T00:20:44Z

packages/firestore/.eslintrc.json

+            "**/test/**/*.ts"
+        ],
+        "rules": {
+            "no-console": "off"


There is a few places where it seems we use console.log intentionally, so I decided to just allow it.
I will enable "no-console" for test related files and add suppression instead. If anything of them should actually be removed, let me know, then I can remove them.

packages/firestore/src/api/database.ts

packages/firestore/src/api/field_value.ts

packages/firestore/src/core/sync_engine.ts

packages/firestore/src/local/persistence_promise.ts

packages/firestore/src/platform/config/goog_module_config.ts

packages/firestore/test/integration/bootstrap.ts

packages/firestore/test/unit/local/local_store.test.ts

packages/firestore/test/unit/remote/remote_event.test.ts

packages/firestore/src/platform_browser/webchannel_connection.ts

mikelehen · 2019-06-21T17:17:22Z

integration/shared/validator.js

+        // __expect(candidate.SDK_VERSION).to.equal(
+        //   require('../../packages/firebase/package.json').version
+        // );
+        __expect(true).to.equal(true);


mikelehen · 2019-06-21T17:52:16Z

packages/firestore/package.json

@@ -32,6 +32,8 @@
    "@firebase/logger": "0.1.17",
    "@firebase/webchannel-wrapper": "0.2.21",
    "@grpc/proto-loader": "^0.5.0",
+    "@firebase/util": "0.2.20",
+    "protobufjs": "6.8.8",


Hrm. Technically I think a direct dependency on protobufjs is only needed for tests... what led you to move it?

As a sidenote: grpc and protobuf stuff are only relevant to node. I guess we rely on bundlers to drop the unused dependencies on web?

It's used in https://github.com/firebase/firebase-js-sdk/blob/master/packages/firestore/src/platform_node/load_protos.ts#L21

This rule catches any import that's not in the right dependency list.

Yep, it won't affect the web bundle.

I see... the actual code that uses the import is only called from tests though:

firebase-js-sdk/packages/firestore/src/platform_node/load_protos.ts

Line 55 in cbdc789

export function loadRawProtos(): ProtobufJS.Root {

So maybe we should suppress the rule, perhaps with a TODO to try to clean things up (it's admittedly a bit hacky).

Done. suppressed the rule with a comment.

packages/firestore/test/integration/bootstrap.ts

mikelehen · 2019-06-25T15:59:29Z

FYI- Please assign back to me when it's ready for a (hopefully) final pass... Though it looks like there's a merge conflict to resolve, so maybe do that first :)

mikelehen

LGTM with one nit (sorry!).

mikelehen · 2019-06-26T00:22:46Z

packages/firestore/test/unit/remote/remote_event.test.ts

+interface PendingTargetResponses {
+  [targetId: string]: number;
+}
+/* eslint-enable  @typescript-eslint/prefer-interface */


Good catch! removed.

Feiyang1 requested review from hiranya911 and hsubox76 as code owners June 8, 2019 00:48

Feiyang1 assigned mikelehen and Feiyang1 and unassigned mikelehen Jun 8, 2019

Feiyang1 changed the title ~~migrate Firestore to eslint~~ migrate Firestore to eslint (WIP) Jun 8, 2019

schmidt-sebastian reviewed Jun 8, 2019

View reviewed changes

Feiyang1 changed the title ~~migrate Firestore to eslint (WIP)~~ migrate Firestore to eslint Jun 10, 2019

Feiyang1 assigned mikelehen and schmidt-sebastian and unassigned Feiyang1 Jun 10, 2019

Feiyang1 force-pushed the fei-eslint-firestore branch from a44260f to 84725bc Compare June 11, 2019 18:59

schmidt-sebastian removed their assignment Jun 17, 2019

mikelehen suggested changes Jun 18, 2019

View reviewed changes

mikelehen assigned wilhuff and Feiyang1 and unassigned mikelehen and wilhuff Jun 18, 2019

Feiyang1 force-pushed the fei-eslint-firestore branch from 84725bc to 2204d79 Compare June 20, 2019 23:41

Feiyang1 commented Jun 21, 2019

View reviewed changes

Feiyang1 assigned mikelehen and unassigned Feiyang1 Jun 21, 2019

mikelehen suggested changes Jun 21, 2019

View reviewed changes

mikelehen assigned Feiyang1 and unassigned mikelehen Jun 21, 2019

Feiyang1 force-pushed the fei-eslint-firestore branch from d0ba151 to 2b50685 Compare June 22, 2019 00:02

hsubox76 approved these changes Jun 25, 2019

View reviewed changes

Feiyang1 added 19 commits June 25, 2019 17:04

migrate Firestore to eslint

71f6e1c

[AUTOMATED]: Prettier Code Styling

ca4d4ec

replace tslint suppresion with eslint ones

92e04b1

disable no used var for params in firestore

3451ae5

[AUTOMATED]: Prettier Code Styling

6976753

enable no-console for test files

1e83960

disable no-floating-promises for test files

eb9fe81

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

f458b0f

address mike's comments

2d4fd4b

[AUTOMATED]: Prettier Code Styling

8e5c68e

move comments to the same line as the suppression

555119a

[AUTOMATED]: Prettier Code Styling

4ef6600

remove unneeded comment

44cf6d1

revert local changes

d6c38c7

enable no-explicit-any in test files

939e754

fix lint issues after rebasing with master

5ca728b

[AUTOMATED]: Prettier Code Styling

580bf03

move protobufjs to devdeps

bc5cc65

[AUTOMATED]: Prettier Code Styling

0f25fdf

Feiyang1 force-pushed the fei-eslint-firestore branch from 37729fb to 0f25fdf Compare June 26, 2019 00:07

Feiyang1 assigned mikelehen and unassigned Feiyang1 Jun 26, 2019

mikelehen approved these changes Jun 26, 2019

View reviewed changes

mikelehen assigned Feiyang1 and unassigned mikelehen Jun 26, 2019

remove leftover suppression

540bbef

Feiyang1 merged commit 7043422 into master Jun 26, 2019

Feiyang1 deleted the fei-eslint-firestore branch June 26, 2019 18:28

firebase locked and limited conversation to collaborators Oct 10, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

migrate Firestore to eslint #1859

migrate Firestore to eslint #1859

Feiyang1 commented Jun 8, 2019

schmidt-sebastian Jun 8, 2019

Feiyang1 Jun 10, 2019

schmidt-sebastian Jun 10, 2019

mmermerkaya Jun 20, 2019

schmidt-sebastian Jun 8, 2019

Feiyang1 Jun 10, 2019

mikelehen Jun 10, 2019 •

edited

Loading

Feiyang1 Jun 11, 2019 •

edited

Loading

mikelehen Jun 11, 2019

Feiyang1 Jun 11, 2019

Feiyang1 commented Jun 14, 2019

mikelehen left a comment

mikelehen Jun 18, 2019

mikelehen Jun 18, 2019

Feiyang1 Jun 20, 2019

Feiyang1 left a comment •

edited

Loading

Feiyang1 Jun 20, 2019

mikelehen Jun 21, 2019

Feiyang1 Jun 21, 2019

mikelehen Jun 21, 2019

Feiyang1 Jun 21, 2019 •

edited

Loading

mikelehen Jun 22, 2019

Feiyang1 Jun 24, 2019

mikelehen commented Jun 25, 2019

mikelehen left a comment

mikelehen Jun 26, 2019

Feiyang1 Jun 26, 2019

migrate Firestore to eslint #1859

migrate Firestore to eslint #1859

Conversation

Feiyang1 commented Jun 8, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mikelehen Jun 10, 2019 • edited Loading

Choose a reason for hiding this comment

Feiyang1 Jun 11, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Feiyang1 commented Jun 14, 2019

mikelehen left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Feiyang1 left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Feiyang1 Jun 21, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mikelehen commented Jun 25, 2019

mikelehen left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mikelehen Jun 10, 2019 •

edited

Loading

Feiyang1 Jun 11, 2019 •

edited

Loading

Feiyang1 left a comment •

edited

Loading

Feiyang1 Jun 21, 2019 •

edited

Loading