introduce ESLint into the build workflow

[43081j]

Are you interested in this?

Basically you already had your ESLint config in the repo and what not, so I:

• Added the correct dependencies
• Switched to the typescript eslint parser
• Ignored *-css.ts (these look like build artifacts?)
• Ignored *.d.ts (again from builds?)
• Disabled a few rules for *.ts (see below)
• Set stylistic rules (indent and max-len) to warn (see below)

Disabled rules

I disabled the following for ts files:

• no-invalid-this because we very often call this in observer functions (which eslint doesn't detect as being bound)
• no-unused-vars because we have compiler options enabled to handle this (more accurately)
• new-cap because we often call something like new this.mdcFoundation.foo

Stylistic rules

I set indent and max-len to warnings because they should ideally be turned off and Prettier (or clang-format) introduced.

The gts repo switched over to prettier a while back iirc, so maybe we should do the same. I'm happy to take their config and add prettier here, then run it over the source.

Commits

I committed 5 things separately in case we do or don't want particular parts:

• Introducing ESLint
• Updating the ESLint config
• Fixing lint errors (but not warnings, i.e. the above mentioned stuff)
• Introduce eslint-plugin-wc for web component specific rules (this successfully picked up (bug) document.createElement('mwc-tab') throws Uncaught DOMException: Failed to construct 'CustomElement': The result must not have attributes #276 for example)
• Add linting to the build script

cc @azakus @sorvell @frankiefu

[stramel]

@azakus @e111077 Any thoughts on getting this in? I'm sure @43081j would update this if you all would be willing to merge.

Might be good to add his lit plugin as well?

[e111077]

@azakus what do you think? This is material's setup here:

https://github.com/material-components/material-components-web/blob/master/.eslintrc.yaml

[43081j]

i rebased in case you ever do want this.

let me know

[stramel]

Friendly ping on this @azakus

[aomarks]

+1

One question though -- our projects are mostly using tslint. I know tslint has ceded to eslint long term, but I'm not up to speed on how that's been going. @43081j @stramel any thoughts on the parity between tslint and eslint today?

I'd also be in favor of clang-formatting everything.

[43081j]

TSLint was always behind in that it was a standalone project rather than an extension.

• ESLint has always had more rules
• Most tslint rules are available in ESLint (via the typescript plugin or in the core)
• The recommended configs differ

There is some compatibility config and/or plugin included to match tslint more closely. But really you're better off just starting again (usually just inherit the Google eslint config and you're ready).

So the main difference is in what the base/reconnected config contains. Recently they even introduced rules which make use of the type system, so almost all tslint rules are covered now.

[abdonrd]

Oh! I just saw this issue!
A few days ago I created this one: lit/lit#967 (Draft PR: lit/lit#969)

[43081j]

@aomarks i did a run of clang-format (the one from latest npm i clang-format) and weirdly ended up with it replacing things like:

import {
  A,
  B,
  C
} from 'foo';

// with

import {A, B, C,} from 'foo';

but on super long lines, and even leaving the trailing comma in 🤔

[stramel]

I think it would be good to put out another PR for clang-format and maybe include husky and lint-staged similarly to what @abdonrd recommended in lit-html.

[aomarks]

@aomarks i did a run of clang-format (the one from latest npm i clang-format) and weirdly ended up with it replacing things like:

import {
  A,
  B,
  C
} from 'foo';

// with

import {A, B, C,} from 'foo';

but on super long lines, and even leaving the trailing comma in

If I run with the same .clang-format we use in lit-html (https://github.com/Polymer/lit-html/blob/master/.clang-format), then it keeps all the imports on one line like before.

[43081j]

@aomarks i figured it out..

i copied the npm script from lit-html (and the clang-format file you guys always use):

    "format": "clang-format --version; find packages | grep '\\.js$\\|\\.ts$' | xargs clang-format --style=file -i",

but here, the first clang-format comes from npm (i.e. npx clang-format) whereas the piped one comes from the system. in my case, im on an ubuntu which has no modern version and produces different output!

good times. not sure what to do about that, probably just manage and try get the new version for myself

[aomarks]

@aomarks i figured it out..

i copied the npm script from lit-html (and the clang-format file you guys always use):

    "format": "clang-format --version; find packages | grep '\\.js$\\|\\.ts$' | xargs clang-format --style=file -i",

but here, the first clang-format comes from npm (i.e. npx clang-format) whereas the piped one comes from the system. in my case, im on an ubuntu which has no modern version and produces different output!

good times. not sure what to do about that, probably just manage and try get the new version for myself

If we add clang-format as a dev-dependency, then npm run format will always do the right thing, right?

I think maybe this is a good command to use on this repo, so that we include test files, but exclude node_modules (you can also make find not even try to traverse into any node_modules/ dir, but I always find the syntax confusing):

find . -name '*.ts' | grep -v node_modules | xargs clang-format -i

[43081j]

no because the end clang-format isn't from npm.

you're xargs-ing it (unix) so you're piping to the system clang-format (as in /usr/bin/clang-format).

im not sure how we could use the bundled one in combination with a find and grep. maybe xargs npx clang-format -i?

[aomarks]

I think it would be good to put out another PR for clang-format and maybe include husky and lint-staged similarly to what @abdonrd recommended in lit-html.

Since the lint and clang-format seems to be pretty much done in this PR already, I say we just submit this PR, and then anyone is welcome to send a followup to add husky (seems good, but need to check with the team a bit, since we haven't used it much before).

[aomarks]

no because the end clang-format isn't from npm.

you're xargs-ing it (unix) so you're piping to the system clang-format (as in /usr/bin/clang-format).

im not sure how we could use the bundled one in combination with a find and grep. maybe xargs npx clang-format -i?

Hm, I thought that npm run adds the local node_modules/.bin to the $PATH so that this should work? I just tried adding a command that just does echo $PATH and that seemed to be the case.

[43081j]

yep you are right @aomarks

im not sure whats going on locally then in that case, ill open a new pr