Move to eslint with idempotent configuration & ensure lint in CI #1475

tchakabam · 2017-12-23T05:19:58Z

Description of the Changes

Ok 😄 Let's improve the code style of our great project! ❤️

All these changes have been done with the --fix option of eslint - it's as easy as that.

We have been setting up almost the same rules for Clappr, with the main difference that it forbids semi-colon, whereas we are are requiring it.

Wether we use one or the other convention (also for any other thing except semi-colons) I don't have a too strong opinion. So all of the rules I set up are subject to change or discussion right here! 👐

But I would love if we could leave behind many of the current inconsistencies and hard-to-read-parts we have. Part of it can be fixed if we tie our code to a powerful tool like eslint and more "strict" rules ✅ :)

My main goals are:

Object litterals need to be "nice", especially since we use a lot of them
Indentation should be strictly consistent
Comma-spacing, Semi-spacing, Bracket-spacing, Keyword-spacing should be strictly consistent
Test code must follow same rules as src

Nice:

It would be nice to impose const when variables aren't mutated
It would be nice to get rid of var
...to get rid of any kind of multi-var declarations

Also I recommend keeping warnings about code block nesting-depth, and trying to minimize them.

Usually it indicates our control structures could be "flatened" using functions, or also reducing redundancy.

High nesting-depth increases complexity and interdependency, less helps testing independent functions more easily.

HOW TO DEAL WITH THIS PR

Let's agree on a style first ✊
Only then, we re-play all the necessary changes from the master head at that point using the eslint --fix script
Fix only a few remaining issues by hand
Enjoy beautiful code

CheckLists

changes have been done against master branch, and PR does not conflict
no commits have been done in dist folder (we will take care of updating it)
new unit / functional tests have been added (whenever applicable)
Travis tests are passing (or test results are not worse than on master branch :))
API or design changes are documented in API.md

NicolasSiver

PR is humongous. Can we introduce eslint step-by-step?

NicolasSiver · 2017-12-24T01:04:32Z

.eslintrc.js

@@ -0,0 +1,63 @@
+module.exports = {


The preferable way to split configs. One should be part of the source es6, module, browser, other would be in tests and could be commonjs, node etc.

NicolasSiver · 2017-12-24T01:05:19Z

.eslintrc.js

+      // Build globals
+      "__dirname": false,
+      // Test globals
+      "after": false,


It's weird, that global functions of mocha should be specified in eslint config.

why is that weird? :)

we could of course only have them in the config inside tests

NicolasSiver · 2017-12-24T01:08:59Z

package.json

@@ -35,7 +35,8 @@
    "pretest": "npm run lint",
    "test": "karma start karma.conf.js",
    "testfunc": "cross-env BABEL_ENV=test mocha --compilers js:babel-register tests/functional/auto/hlsjs.js --timeout 40000",
-    "lint": "jshint src/",
+    "lint": "eslint src/ tests/",
+    "lint:fix": "eslint src/ tests/ --fix",


Don't look necessary.
How often do we plan to use fix command? I assume only once - as a part of this PR. Also: npm run lint -- --fix is the same.

well actually it is super useful during development, most mistakes which happen for me are auto-fixable, so it really makes it super easy to keep your commits lint-correct :)

but then maybe you are using an editor that already support eslint auto-fixes and so you wouldn't need that.

anyhow, how bad is having such a script that can be useful here? :)

NicolasSiver · 2017-12-24T01:12:08Z

src/config.js

-  manifestLoadingTimeOut: 10000,          // used by playlist-loader
-  manifestLoadingMaxRetry: 1,             // used by playlist-loader
-  manifestLoadingRetryDelay: 1000,        // used by playlist-loader
+  autoStartLoad                 : true,                    // used by stream-controller


There is an issue with comments.
I would put them on the previous line, i.e.:

// used by stream-controller autoStartLoad: true

Also, comments do not feel really used in a valuable way. If it's hard to track config property use, it should be re-thought.

first of all, i would say it would be good to add more JSdoc in there. it's not really useful to know where it's used, but it should be more documented what it's for :)

and also, i want to know where it's used, i can just text-search it.

and well i guess the proper way would be to actually structure the config into sub-domains per concern or self-containance of components i guess :)

but this PR isn't about improving the config API, however that is also something to think about of course.

+1 for putting comments on the next line

NicolasSiver · 2017-12-24T01:13:37Z

src/config.js

-  maxStarvationDelay : 4,                   // used by abr-controller
-  maxLoadingDelay : 4,                      // used by abr-controller
-  minAutoBitrate: 0                         // used by hls
+  fLoader                       : undefined,


null is better than undefined, in a way, that it signifies that we know about the property but it's unset.

yes totally agree, let's ban undefined, it's nothing one should ever need to use.

typeof foo === ... covers anything one ever needs to do. instead of checking for undefined, let's better check for the type we actually expect.

tchakabam · 2018-01-04T15:21:49Z

@NicolasSiver

PR is humongous. Can we introduce eslint step-by-step?

How do you wanna do that?

One way could be to set up the config etc, and come up with all the rules we want.

But then, why not also apply them straight away? It will all be iso-functional.

I would greatly appreciate to work on a more "beautiful" code-base really, and I think it will be beneficial to the project all-over - and bring us up to par with the general standard of good quality open-source code. our rules here aren't even so strict.

of course lint rules isn't everything, there is also documentation and generally the structure of functional modules etc, but imho it's something we should have to make it more readable and accessible.

also, i think it would be really overall helpful for readability if more composite words were actually written proper camel-cased. so not have things like foobarhouse but at least foobarHouse, if fooBarHouse, and no more a=b.thing; b.otherthing={things,morethings:abc} in one line and so on :)

tchakabam · 2018-01-04T15:23:02Z

@NicolasSiver to answer your question: yes agreed that we should not commit the rules applied straight away. let's figure out the rules, agree on that, and when we re all sure about it, we do one eslint --fix and commit that. or did you mean something else?

tchakabam · 2018-01-05T12:48:28Z

stashed all the code changes for now, just eslint-config and script changes.

johnBartos · 2018-01-05T18:59:23Z

What do we think of prettier?

https://github.com/prettier/prettier

It'll do all the formatting for us, at the cost of an opinionated styleguide.

johnBartos · 2018-01-05T19:08:53Z

#1485

tchakabam · 2018-01-08T18:41:40Z

@johnBartos opinionated sounds like "no config" which is nice, but on the other hand i think it's good to be able to choose the config :)

but what are the advantages of prettier vs eslint?

and is it at all a full replacement for eslint?

eslint also can apply the fixes for you, with the --fix option automatically, or thus integrate with your editor and such. just that it actually allows itself to be configured.

eslint is the de-facto standard linting/formatting tool on JS projects. many people know it, there are common rule-sets. it has proven to work well. why a new tool?

tchakabam · 2018-01-08T18:45:51Z

@johnBartos for example what i miss in "prettier" is actual formatting details. the example on the webpage is ridiculously ultra-simple. there are so many cases and open questions it doesn't cover. where is their styleguide?

with eslint we can also follow "some opinion" but at least, that will be detailed. for example the so-called "node-style-guide" which is what for example the Clappr code-base extends their config from with a few modifications.

https://github.com/felixge/node-style-guide

you will see all of this style-guide is much more detailed.

eslint allows to import & extend/overload existing configurations. so if you want something opinionated, we can do it.

dighan · 2018-01-08T18:55:16Z

That's exactly the point. Prettier is like gofmt: avoids infinite debates about coding style. Developers can then focus on what really matters, the code.

tchakabam · 2018-01-08T19:02:15Z

sure, but i just think that eslint allows much more strict rules that imo are necessary.

where is the prettier style guide? on their page is only a ridiculously simple example.

tchakabam · 2018-01-08T19:04:53Z

also, the main thing is: Prettier does not replace linter.

They say it themselves: https://prettier.io/docs/en/comparison.html

They don't have actual code-quality rules. That's one problem i see with it.

a linter finds problems in your code, like block depth, or unused variables, or usage of console.log, etc etc. and you can configure it to what it should do exactly depending on your needs :)

tchakabam · 2018-01-08T19:06:50Z

@dighan and eslint can work same as gofmt and format your code, with the --fix option, as there are many rules that are "auto-fixable". eslint knows do what prettier does, just better and more mature imo.

tchakabam · 2018-01-08T19:11:40Z

one option for using prettier would be using this: https://github.com/prettier/eslint-config-prettier

then we can still use eslint for code quality rules, and let prettier do the formatting.

however, i think it is weird to have two tools, and having to disable options of one on purpose to be able to use the other :D

but ok.

my main goal here is to have some kind of nice format in the code, as i said before, i am not too picky on the exact rules. so maybe the prettier thing will do a good job too, even if i would still like to see a little bit more documentation on the "style opinion"

tchakabam · 2018-01-08T22:19:57Z

one other advantage of eslint is incremental and scoped configuration. for example you can have a main config in project root, and a nested config in "tests" folder, where you declare all the globals needed for your test framework.

…e-code-style

benjamingr · 2018-01-09T10:43:30Z

I think that this should be iterative - where the lint is applied to files touched as they're touched (as a commit hook).

tchakabam · 2018-01-09T10:46:29Z

@benjamingr That's a pretty good idea :) 👍

tchakabam · 2018-01-10T21:36:01Z

@benjamingr i am not sure how to run the linter only over only files in the git stage area however ... :)

johnBartos · 2018-03-02T16:04:17Z

Summary thus far:

Let's abandon prettier for now. The changes are too drastic.
We all agree on ESLint. We don't all agree on the rules.

After some offline discussion with @tchakabam we both agreed on a PR which introduces ESLint and fixes basic rules (like consistent indentation). Future PRs will be made to fix more opinionated or complex rules which require further buy-in from contributors. Can we all agree on this? @mangui @NicolasSiver @dighan

tjenkinson · 2018-03-02T17:18:13Z

Sounds like a good starting point to me 👍

mangui · 2018-03-02T21:05:13Z

Lgtm

mangui · 2018-03-03T14:37:49Z

This pull request fixes 1 alert when merging e069f52 into fcca4bd - view on lgtm.com

fixed alerts:

1 for Duplicate switch case

Comment posted by lgtm.com

mangui · 2018-03-03T14:51:15Z

This pull request fixes 1 alert when merging bc0bfa7 into fcca4bd - view on lgtm.com

fixed alerts:

1 for Duplicate switch case

Comment posted by lgtm.com

mangui · 2018-03-03T15:42:53Z

This pull request fixes 1 alert when merging 490d28b into fcca4bd - view on lgtm.com

fixed alerts:

1 for Duplicate switch case

Comment posted by lgtm.com

mangui · 2018-03-03T15:56:58Z

This pull request fixes 1 alert when merging 9090233 into fcca4bd - view on lgtm.com

fixed alerts:

1 for Duplicate switch case

Comment posted by lgtm.com

…on code checking (warnings only)

mangui · 2018-03-05T10:05:31Z

This pull request fixes 1 alert when merging 6103c9c into 8fc43ef - view on lgtm.com

fixed alerts:

1 for Duplicate switch case

Comment posted by lgtm.com

mangui · 2018-03-05T10:32:04Z

This pull request fixes 1 alert when merging 47dbd37 into 8fc43ef - view on lgtm.com

fixed alerts:

1 for Duplicate switch case

Comment posted by lgtm.com

tchakabam · 2018-03-05T11:03:48Z

Hey guys,

So I think this is done finally. I hope you understand I also need to time-box the effort on this.

From my side, I don't really need to have a discussion on specific rules or style either in fact :D

Which is why we can go with the Standard JS style (https://standardjs.com/), which is a well-documented and established specification.

But that's not why I was opposed at using Prettier. Style-details are not really the issues here imho. Please read on for more explanations on why ESlint, and what we do exactly here.

Meanwhile, I would really love to get this merged soon (before having other PRs cause conflicts hehe) and move on with a code base that we can work with confidently, and embrace all the good contributions we get! This is important I think.

🚧 Current WIP PRs should pull in master after this is merge, and just apply the lint:fix script, or fix issues they already have in their commits as needed. 🚧

About this PR:

There aren't any functional changes in here at all, but only good style fixes and a few actual code flaw issues. So I hope you can be confident with this, without checking every single line of this huge diff. Basically the diff is from the eslint --fix option that will correct errors automatically where possible. A few little things needed to be fixed by myself, where we were relying on var hoisting for example (resulting in linter saying that the variable is declared after usage). One other nice one: We had a duplicate case statement in a switch-block inside StreamController 🙈

Also, the auto-testbench code that was embedded in the HTML is now moved into a seperate JS file where the linter can find it. No changes otherwise, only that we are explicitely setting all things on the
window so that we don't need to rely on "globals" in the test-runner code :)

Motivations

So I think we have all experienced that with our project we are at a point where we would like to have a better more reliable code quality (see #1593) where all of the commits and the whole codebase get checked through the CI, and also enforce a consistent style, and improve the readability and structure of the existing code (less one line litteral object structs in mp4-remuxer for example ;)).

A lint tool should not only ensure consistent and a readable style (which may be adapted to the needs and purposes), but also make sure that there aren't any actual logical flaws and hazards in our code.

That is actually the main reason why I was opposed against a tool like "Prettier" that actually does not take care of the most important aspect of linting, which is static code analysis - regardless of style and readability aspects.

About ESLint

With eslint, we buy into a very high quality, and the now most widely spread linting tool in the JS community. It has also integrations for parsing JS supersets like Flow or TypeScript, or plugins for a wide range of things, as well as pre-configured rulesets, to extend from, and a hierarchical configuration file setup.

Eslint is amazingly well documented with a lot of examples for each rule: https://eslint.org/docs/rules/

Working with ESlint

A few infos on this, how to deal for example with rules which we may want to do exceptions on:

Say we have a rule forbidding empty code block. Now, we may actually need an empty code block in a specific place.

To disable a single line for example, use then // eslint-disable-line no-empty

Or, if you want to disable the next line, use // eslint-disable-next-line no-empty

Or if you want to add a global var in a file (even if it should not really be necessary ideally): /* global MyVar */

For globals that are potentially present in the whole code-base, the eslint config file at the respective root of the file tree can be used in the "globals" section.

For the tests we have a eslint sub-config that extends the config at the project root, which contains globals needed by moch test-runner for example.

If you want to disable a rule in a whole file, use for example /* eslint-disable no-unused-vars */

A lot of stuff can be done with it, but other than that it just works :)

Many rules, especially concerning style can be "auto-fixed".

For this we can add the --fix option to the eslint call. There is a short-hand for this in our package scripts now (npm run lint:fix).

Obviously there are also plugins for the various editors that integrate with this functionnality, so you basically can write how you want and eslint will fix things after you :)

Other rules, especially concerning code safety/reliability/flaws/limitations need logical decisions from humans to be fixed.

This PR

Introduces ESlint as a tool to enforce code quality standards in Hls.js
Code means as much src as tests :)
Adds running eslint in the CI on all the code, for each build, before running tests
Adds a script to auto-fix "style" rules (lint:fix). No need to spend more time doing it exactly right while coding, this will clean-up after you :)
Adds a "precommit" hook, which will enforce running a linter before each commit (so you don't have to wait for the CI to tell you and can make sure what you push is clean) - pre-commit hooks can be overriden by git commit -n if you need to.
One script to run lint on lint:src, one to run lint on lint:tests (one run can take a bit with our codebase) and a lint:fix script that fixes all possible auto-fixable issues, and lint which runs both on src and tests.
Auto-functional tests now have minimal code in HTML and all the testbench code gets linted :)
We have source-maps for all the webpack builds (not really related but seemed necessary to actually get a useful stacktrace from auto-functional tests)
Fixes all lint issues for the rules that are set up. Most automatically fixable, a few manual fixes.
** var hoisting
** duplicate switch case
** and more ...

These are:

eslint:recommended, that is a pre-configured ruleset with the basic code safety rules. See https://eslint.org/docs/user-guide/configuring#using-eslintrecommended

It doesn't make sense having a linter without enabling these really :)

standard: This project is basically like Prettier, only that it exists since a while, the idea is to have a consistent, meaningful, functional, good JavaScript style that can be dropped into a project without many discussions: https://standardjs.com/

The main difference being, Standardjs don't enforce a tool, but define the style clearly and imho with good ideas in mind, while integrating it with existing tools like ESlint.

This means we pull-in the eslint-config-standard via npm, and can then tell eslint to extend our config from this.

See https://github.com/standard/eslint-config-standard

Our "Hls.js" style:

Base:

2 spaces indent (no tabs ever)
Always semi-colons
Single quotes
Unix line-break
Regular spacing on infix and unary operations
No space in parenthesis
Enforce litteral objects on multiple lines (improves greatly readability and clarity on data structures!!) :)

Warnings (which we can set to errors if we want):

Exerpt from the config, you can check yourself what these are about.

Main concern here is code that is safe, doesn't go into spagethi switch-cases, doesn't use undefined, has a regular file structure, has capitalized constructors, etc etc.

If you break these rules your commit will still go through, but it would be nicer to not do that. And maybe we can discuss to make these rules errors in the future.

    "standard/no-callback-literal": 1,
    "import/first": 1,
    "no-var": 1,
    "no-empty": 1,
    "no-mixed-operators": 1,
    "no-unused-vars": 1,
    "no-console": 1,
    "no-fallthrough": 1,
    "no-case-declarations": 1,
    "no-irregular-whitespace": 1,
    "no-self-assign": 1,
    "new-cap": 1,
    "no-undefined": 1

A few more rules we could make warnings, that are mostly concerned with maximum nesting of code blocks and callbacks, but also line lenght and amount of statements per block.

Giving these rules a meaningful configuraton will definitely make sure we write functions and logic in manageable ways, and get a warning when we don't. Usually when a certain depth of nesting is exceeded, the developers should think why this is so, and flatten it out.

    "max-len": 1,
    "max-statements": 1,
    "max-depth": 1,
    "max-nested-callbacks": 1,
    "max-params": 1,
    "max-statements": 1,
    "max-statements-per-line": 1

Note not only the "style" rules are highly customizable and configurable, check the documentation on that.

Main concern is that we end up with a higher quality super reliable, beautiful, smart and extendable code base! 👍

Currently, we get a lot of "warnings". If you have ever programmed C or C++ before, this may sound familiar too you. Point is, it is much better to have a picky tool shouting at our code, then not :D

Cheers! 🍻

`

tchakabam · 2018-03-05T11:06:22Z

Eslint root config: https://github.com/video-dev/hls.js/blob/47dbd371af86b70facc9e3231db19df98cf0d8a9/.eslintrc.js
Eslint: tests config: https://github.com/video-dev/hls.js/blob/47dbd371af86b70facc9e3231db19df98cf0d8a9/tests/.eslintrc.js
Package.json: https://github.com/video-dev/hls.js/blob/47dbd371af86b70facc9e3231db19df98cf0d8a9/package.json
Precommit: https://github.com/video-dev/hls.js/blob/47dbd371af86b70facc9e3231db19df98cf0d8a9/scripts/precommit

…ndow

mangui · 2018-03-05T11:53:42Z

This pull request fixes 1 alert when merging c3fd24c into 8fc43ef - view on lgtm.com

fixed alerts:

1 for Duplicate switch case

Comment posted by lgtm.com

mangui

in favor of merging that early for project consistency.

tchakabam · 2018-03-06T12:56:35Z

let's get back to work on awesome features and a great library 👍

NicolasSiver requested changes Dec 24, 2017

View reviewed changes

tchakabam added 5 commits January 5, 2018 13:46

add .eslintrc.js

2d358a5

add eslint deps

ac38981

rm jshint deps

7328de2

update lint scripts

b5e9cd7

rm jshintrc

5100cd8

tchakabam force-pushed the lets-improve-code-style branch from bf6b1d0 to 5100cd8 Compare January 5, 2018 12:47

tchakabam added 4 commits January 9, 2018 04:16

add prettier deps

3a5c561

add prettier script, and improve namings of scripts

7aebaa1

add prettier to eslint config

4b7b767

Merge branch 'master' of github.com:video-dev/hls.js into lets-improv…

c14af8a

…e-code-style

tchakabam mentioned this pull request Jan 9, 2018

[Do Not Merge] [Request for Comments] Format everything with prettier #1485

Closed

5 tasks

tchakabam added 2 commits January 10, 2018 23:49

set up pre-commit hook

5699ef8

add precommit scripts that lints only staged files

80afa2a

tchakabam added 5 commits March 5, 2018 10:09

eslint rules based on recommend and standard rules, a few relaxtions …

07849c8

…on code checking (warnings only)

webpack config: create source-map for all builds

577551e

package scripts

0dbcb77

foldersize script: add shebang line

5d65f07

precommit script: run linter

e3ea0a8

tchakabam force-pushed the lets-improve-code-style branch from 9090233 to e3ea0a8 Compare March 5, 2018 09:17

tchakabam added 3 commits March 5, 2018 10:53

final lint rules for basic code quality, make one-var rule warning

974da0f

fix all auto lint errors

4e4b2ab

Merge branch 'master' into lets-improve-code-style

6103c9c

auto functional tests setup fixes

47dbd37

tchakabam added 3 commits March 5, 2018 12:42

no let in auto testbench (script src, no babel)

06d62dd

auto test html should call from console log redirection setup from wi…

48bf64a

…ndow

remove obsolete comment in eslintrc

c3fd24c

tchakabam removed the Work in Progress label Mar 6, 2018

mangui approved these changes Mar 6, 2018

View reviewed changes

tchakabam merged commit f44dd24 into master Mar 6, 2018

tchakabam deleted the lets-improve-code-style branch November 20, 2018 14:55

Move to eslint with idempotent configuration & ensure lint in CI #1475

Move to eslint with idempotent configuration & ensure lint in CI #1475

Conversation

tchakabam commented Dec 23, 2017 • edited

Description of the Changes

CheckLists

NicolasSiver 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

Choose a reason for hiding this comment

tchakabam Jan 10, 2018 • edited

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tchakabam commented Jan 4, 2018

tchakabam commented Jan 4, 2018

tchakabam commented Jan 5, 2018

johnBartos commented Jan 5, 2018

johnBartos commented Jan 5, 2018

tchakabam commented Jan 8, 2018 • edited

tchakabam commented Jan 8, 2018 • edited

dighan commented Jan 8, 2018

tchakabam commented Jan 8, 2018

tchakabam commented Jan 8, 2018 • edited

tchakabam commented Jan 8, 2018 • edited

tchakabam commented Jan 8, 2018

tchakabam commented Jan 8, 2018

benjamingr commented Jan 9, 2018

tchakabam commented Jan 9, 2018

tchakabam commented Jan 10, 2018

johnBartos commented Mar 2, 2018

tjenkinson commented Mar 2, 2018

mangui commented Mar 2, 2018

mangui commented Mar 3, 2018

mangui commented Mar 3, 2018

mangui commented Mar 3, 2018

mangui commented Mar 3, 2018

mangui commented Mar 5, 2018

mangui commented Mar 5, 2018

tchakabam commented Mar 5, 2018 • edited

Motivations

About ESLint

Working with ESlint

This PR

tchakabam commented Mar 5, 2018

mangui commented Mar 5, 2018

mangui left a comment

Choose a reason for hiding this comment

tchakabam commented Mar 6, 2018

tchakabam commented Dec 23, 2017 •

edited

tchakabam Jan 10, 2018 •

edited

tchakabam commented Jan 8, 2018 •

edited

tchakabam commented Jan 8, 2018 •

edited

tchakabam commented Jan 8, 2018 •

edited

tchakabam commented Jan 8, 2018 •

edited

tchakabam commented Mar 5, 2018 •

edited