eslint rules for firebase projects #1831

Feiyang1 · 2019-05-30T06:54:39Z

Create the base eslint rules for firebase projects. We will apply it to all repos eventually.
As the first step, we need to agree on the set of rules, so feedback is welcomed.

I have run this set of rules against all projects, and there are about 4000 issues. Given the large number of issues, we will enable the eslint rules for project one by one in a few followup PRs.

The set of rules derives from the google tslint rules. I made a document to show what rules from google tslint rules are included/excluded and what additional rules are included in this PR.

mmermerkaya · 2019-05-30T10:36:06Z

Is typescript-eslint feature complete? Shouldn't we wait until they deprecate TSLint, which they will do after ESLint is feature complete?

You can see what's implemented and what's not here.

Also I recommend using eslint-config-prettier and eslint-plugin-prettier instead of defining style rules. Or if we do TSLint instead, there's TSLint versions of these two as well. Example here.

Feiyang1 · 2019-05-31T22:22:26Z

typescript-eslint is not feature complete according to the roadmap you linked, but it has implemented the majority of the features and a lot of the tslint rules have an equivalent rule in eslint already.

For the tslint rules that are not available in eslint/@typescript-eslint yet, I'm using @typescript-eslint/tslint to fill the gap. We will remove this plugin eventually as the features are implemented in eslint/@typescript-eslint.

I don't think we need to include any code formatting related rules in eslint, since it will be dealt with by prettier. I also don't want to fail compilation because of the formatting issue.

mmermerkaya · 2019-05-31T22:48:36Z

I don't think we need to include any code formatting related rules in eslint, since it will be dealt with by prettier.

That's exactly what these two do. One disables formatting related rules of ESLint and the other checks Prettier for formatting. So formatting will be dealt with by Prettier, but within ESLint. I think that would make things easier, since we wouldn't have to run two different scripts for prepush and CI tests.

I also don't want to fail compilation because of the formatting issue.

It wouldn't fail compilation, it would fail tests. That's where my packages (messaging and installations) run linters. And I thought CI tests checked formatting already? Or is that only the prepush script?

clutter the output without too much benefits

That is true, but you can run fix / format on save in most editors (including VSCode, which is what I use).

Anyway, I don't feel strongly about any of these, just my two cents.

mmermerkaya · 2019-05-31T22:44:30Z

config/.eslintrc.json

+            "error",
+            "as-needed",
+            {
+                "requireReturnForObjectLiteral": true


But why? 😞 I always tell people that they can do () => ({ ... }) in readability reviews.

I was confused by the syntax the first time when I saw it, but I don't feel strongly about it.

After going through Firestore errors, we are not a fan of "arrow-body-style" at all. We have 566 violations.
Some of them would be fine to change (though unclear benefit), but some would be worse. For example, it would cause all of our tests to change from, e.g.:

it('can set documents', () => { return integrationHelpers.withTestDoc(persistence, doc => { return doc.firestore .batch() .set(doc, { foo: 'bar' }) .commit() .then(() => doc.get()) .then(snapshot => { expect(snapshot.exists).to.equal(true); expect(snapshot.data()).to.deep.equal({ foo: 'bar' }); }); }); });

to:

it('can set documents', () => integrationHelpers.withTestDoc(persistence, doc => doc.firestore .batch() .set(doc, { foo: 'bar' }) .commit() .then(() => doc.get()) .then(snapshot => { expect(snapshot.exists).to.equal(true); expect(snapshot.data()).to.deep.equal({ foo: 'bar' }); }) ));

We are not in favor of this, since:

We think braces can be a useful tool when coding for denoting long blocks of code (even if it's just chaining method calls).

[quoting @wilhuff] Any multiline block that currently contains a single return statement can end up with multiple statements easily. Refactoring the extra statements in is error prone. This is why we require braces around if statements

So I'd advocate for removing this rule at the Firebase level. And if we keep it, we (Firestore) will likely decide to disable it.

Replaced this rule with tslint "arrow-return-shorthand": true which is currently enabled in firestore.
It enforces arrow function shorthand, but will ignore functions that span multiple lines (this option is not available in the eslint rule arrow-body-style).

mmermerkaya · 2019-05-31T22:53:39Z

config/.eslintrc.json

+        ],
+        "curly": [
+            "error",
+            "multi-line"


Even though it's a bit stricter than Google TS style (doesn't allow braceless single line if), I vote for "all".

I'd be fine with "all". I'm leaving it unresolved for a couple of days to see if anyone objects.

Firestore is a fan of "all" too, thought this will probably create some violations since currently we allow no braces iff the statement is on the same line as the if, e.g.:

if (result.skip) continue;

Looks like eslint doesn't have a similar configuration, so we'll probably lose that... but I think that's okay.

Changed to "all"

mikelehen

Thanks for putting this together! I'm mostly on board but there are a couple rules causing me some concern. :-)

mikelehen · 2019-06-03T18:44:44Z

packages/firestore/package.json

@@ -8,6 +8,7 @@
    "build:console": "node tools/console.build.js",
    "dev": "rollup -c -w",
    "lint": "tslint -p tsconfig.json -c tslint.json 'src/**/*.ts' 'test/**/*.ts'",
+    "lint:eslint": "eslint -c .eslintrc.json '**/*.ts'",


It looks like we need to exclude "dist" somehow.

Added the --ignore-path ../../.gitignore

mikelehen · 2019-06-03T23:44:15Z

config/.eslintrc.json

+        "@typescript-eslint/no-unused-vars": [
+            "error",
+            {
+                "varsIgnorePattern": "^_*$"


I'm not sure if this is working... For Firestore I'm seeing some errors like:

/Users/mikelehen/src/firebase-js-sdk/packages/firestore/src/local/memory_persistence.ts ... 357:60 error '_' is defined but never used @typescript-eslint/no-unused-vars

Added "argsIgnorePattern": "^_".
I didn't realize argsIgnorePattern and varsIgnorePattern are 2 different options util now. :(

mikelehen · 2019-06-04T00:14:22Z

config/.eslintrc.json

+                "allowExpressions": true
+            }
+        ],
+        "@typescript-eslint/no-unused-vars": [


FWIW- Looking through the Firestore errors, this is the other rule we run into a lot. Our current tslint rules prevent unused variables but allow unused arguments, and we have a lot of violations (~170 though some of those are for _, see below), and the ones I've looked at are mostly intentional... Basically there are many places where a function is implementing some contract and we want the function signature to convey the contract, even if the function implementation doesn't need everything provided by the contract.

As a simple example:

return documentsStore .iterate((key, doc) => { const path = new ResourcePath(key); ... });

Here, It so happens that doc isn't needed, but the iterate function will be passed 'key' and 'doc', and it would be confusing to make it look like it's not!

We also have various interfaces with multiple implementations, and not all implementations need all the function arguments provided... but it's still worth explicitly conforming to the contract.

We could suppress all of these, but I wouldn't be excited about adding that kind of clutter... So I would suggest that we make the ignore pattern be something like "^_" and then change all of the intentionally unused parameters to just be prefixed with an underscore, e.g. "_doc" above. That way we can keep the rule, but cleanly mark all the intentional ones without it being too awkward.

WDYT?

Yes! That's the intention, but I made a mistake in the regex (I meant to have ^_.*$). Update the regex to "^_".

mikelehen · 2019-06-04T01:21:35Z

config/.eslintrc.json

+            "error",
+            "as-needed",
+            {
+                "requireReturnForObjectLiteral": true


After going through Firestore errors, we are not a fan of "arrow-body-style" at all. We have 566 violations.
Some of them would be fine to change (though unclear benefit), but some would be worse. For example, it would cause all of our tests to change from, e.g.:

it('can set documents', () => { return integrationHelpers.withTestDoc(persistence, doc => { return doc.firestore .batch() .set(doc, { foo: 'bar' }) .commit() .then(() => doc.get()) .then(snapshot => { expect(snapshot.exists).to.equal(true); expect(snapshot.data()).to.deep.equal({ foo: 'bar' }); }); }); });

to:

it('can set documents', () => integrationHelpers.withTestDoc(persistence, doc => doc.firestore .batch() .set(doc, { foo: 'bar' }) .commit() .then(() => doc.get()) .then(snapshot => { expect(snapshot.exists).to.equal(true); expect(snapshot.data()).to.deep.equal({ foo: 'bar' }); }) ));

We are not in favor of this, since:

We think braces can be a useful tool when coding for denoting long blocks of code (even if it's just chaining method calls).

[quoting @wilhuff] Any multiline block that currently contains a single return statement can end up with multiple statements easily. Refactoring the extra statements in is error prone. This is why we require braces around if statements

So I'd advocate for removing this rule at the Firebase level. And if we keep it, we (Firestore) will likely decide to disable it.

mikelehen · 2019-06-04T01:35:19Z

config/.eslintrc.json

+        ],
+        "curly": [
+            "error",
+            "multi-line"


Firestore is a fan of "all" too, thought this will probably create some violations since currently we allow no braces iff the statement is on the same line as the if, e.g.:

if (result.skip) continue;

Looks like eslint doesn't have a similar configuration, so we'll probably lose that... but I think that's okay.

mikelehen

Thanks! I'm happy with where this has landed.

Feiyang1 requested review from hiranya911 and hsubox76 as code owners May 30, 2019 06:54

Feiyang1 self-assigned this May 30, 2019

mmermerkaya reviewed May 31, 2019

View reviewed changes

add eslint rules

561d3fe

Feiyang1 force-pushed the fei-eslint branch from 752fa16 to 561d3fe Compare June 4, 2019 01:03

mikelehen suggested changes Jun 4, 2019

View reviewed changes

Feiyang1 added 2 commits June 4, 2019 11:25

ignore unused params with a leading _

0c4ff20

update curly and arrow-body-style rules

44256be

Feiyang1 assigned mikelehen Jun 4, 2019

mikelehen approved these changes Jun 4, 2019

View reviewed changes

Feiyang1 assigned alikn Jun 4, 2019

mikelehen removed their assignment Jun 4, 2019

Feiyang1 merged commit bdfcd6c into master Jun 4, 2019

Feiyang1 deleted the fei-eslint branch June 4, 2019 23:15

firebase locked and limited conversation to collaborators Oct 10, 2019

eslint rules for firebase projects #1831

eslint rules for firebase projects #1831

Uh oh!

Conversation

Feiyang1 commented May 30, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mmermerkaya commented May 30, 2019

Uh oh!

Feiyang1 commented May 31, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mmermerkaya commented May 31, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Feiyang1 Jun 1, 2019 • 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!

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!

Feiyang1 Jun 4, 2019 • 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!

mikelehen left a comment

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!

Feiyang1 Jun 4, 2019 • 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!

Choose a reason for hiding this comment

Uh oh!

mikelehen left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Feiyang1 commented May 30, 2019 •

edited

Loading

Feiyang1 commented May 31, 2019 •

edited

Loading

Feiyang1 Jun 1, 2019 •

edited

Loading

Feiyang1 Jun 4, 2019 •

edited

Loading

Feiyang1 Jun 4, 2019 •

edited

Loading