ESLint and Code Style Standardization

[mnaamani]

Attempt to standardize on how we do es linting in the monorepo and apply code style for js/ts packages

• Reusable eslint, prettier and tsconfig config package
• Update the workspaces' eslintc file to use @joystream/eslint-config (while still extending) - change to use .eslintrc.js not .eslintrc
• Add/Update npm scripts to execute linting (on master for now cli linter is run with the posttest script, there are a lot of linter errors currently)
• Change network-tests workspace to use eslint, currently it uses tslint
• Test prettier - make sure eslint and prettier play nice
• testing in VS Code

Apply linting and prettier fixes will be responsibility of maintainer of each package.

Some notes/guidlines on

code style and formatting
There is now one prettier config at the root which should be used by all projects. So you should avoid adding any prettier or editorconfig files anywhere else (prettier will traverse from the starting point of where you run prettier and stop at the first config file it finds)
All style related eslint rules are disabled, and the prettier plugin is configured to make style/format errors into linter errors so you can fix your code style in the IDE when you have eslint plugin enabled. (I think it means you don't need to explicitly also have the prettier extension enabled in the IDE).

If you enable "format on save" setting in your IDE with prettier plugin enabled, it should work. You probably want to re-format and fix linter warnings in one PR at some point before adding more code per project to make PRs simpler to review.

yarn prettier . --write

will format all the files in the current directory.

Linting:
We should all now use eslint and there is a config file at the root with a slim set of rules. Basically just the recommended.
The parser used is @typescript-eslint/parser however the root package.json doesn't add it explicitly for now, so we will get the version that comes with @polkadot/dev, the newer version is breaking pioneer. So will have to bump it when/if we fix all the linter errors there first.

Each project should have its own .eslintrc.js file that will effectively inherit the root config (unless you specify root:true in your config. Please try to avoid doing that, otherwise the whole point of standardizing on eslint rules is pointless). You can still of course override some rules that are important for the specific project type, and add any specific plugins, env configurations appropriate, for example env: { browser: true } if its a web app.

There is a now a "blank" tsconfig at the root (only because eslint was complaining about it missing, I presume because of the eslint config file at the root that expects a ts project) I originally added a reusable tsconfig package that can be used and extended if required but as discussed with @Gamaranto and @kdembler typescript compiler configurations are usually so project specific its not very useful and maybe even a hindrance, so I dropped it. It also doesn't make sense since the root of the monorepo is not a ts project.

Currently the only linting check done as part of CI are for pioneer. Once code in all projects is lint and style fixes we can enable them.

[kdembler]

standard is a styleguide package, we probably should use eslint:recommended instead

[kdembler]

Or alongside if you intended to use standard

[mnaamani]

If I include standard as first rule-set, followed by eslint:recommended, we get benefit of both with the eslint:recommended rules taking precedence over standard?

[mnaamani]

I'm actually going to remove any styleguide packages, because we need prettier for consistent code formatting.

[kdembler]

If I include standard as first rule-set, followed by eslint:recommended, we get benefit of both with the eslint:recommended rules taking precedence over standard?

Yes, that's my understanding.

I'm actually going to remove any styleguide packages, because we need prettier for consistent code formatting.

This makes sense, since prettier eslint plugin will disable any style rules anyway.

[fulminmaxi]

Wouldn't prettier eslint plugin only disable the rules conflicting with prettier? So we would not get all the benefits of the style guide, but still keep relevant checks like eqeqeq

[kdembler]

Check like eqeqeq would be inside eslint:recommended, not standard. We could keep standard however, since looking at the rules it enables (https://github.com/standard/eslint-config-standard/blob/master/eslintrc.json), some may not be covered by prettier since they aren't all code formatting related.

[mnaamani]

yes also I think standard works better for vanilla javascript. Its the standardjs guide if I'm not mistaken

[mnaamani]

* `semi: false` - looks like `network-tests`, Atlas, Pioneer and CLI - all use semicolons, which I think makes the code more clear, so perhaps this should be set to `true`?

:) we sort of came to this decision in our unofficial 'code style' committee meeting with @Gamaranto @kdembler and myself, where basically Klaudiusz and I were the majority. This is similar to tabs vs spaces argument where no team will ever agree, its just important to be consistent. So I can see how we are breaking the consistency rule here since most of the projects are already using semis, which Francesco did point out.

But in all seriousness, with the recommended eslinting rules, I'm assuming the linter will help us catch bugs that could result from unexpected semicolon insertion. If I'm mistaken, then I would be happy to set this rule to true.

Regarding pioneer I was concerned with actually changing formatting too much so we don't get a nightmare when we pull from upstream, but at the same time I don't want pioneer code style to hold us back. We can ofcourse override that rule in the pioneer eslintrc.js and introduce a prettier config just for pioneer to isolate it further.

[kdembler]

Won't checking this file into the repo cause everybody to override it and commit (by mistake for example) their personal preferences?

[fulminmaxi]

I think personal settings are set in the global settings.json and then VSCode takes care of mergin those with the workspace settings, so unless someone actively makes changes to this file, it shouldn't change.

[kdembler]

But people (me for example) may have workspace-specific settings they want to use.

[mnaamani]

Francesco is correct, the workspace settings checked in here will only override your global settings.
So you personal preferences can go into the User global settings.
If you have some good workspace settings then the rest of the team could benefit also by adding them here. What do you think?

[kdembler]

Francesco is correct, the workspace settings checked in here will only override your global settings.

Well, I'm not sure that's a good thing. For example, someone (from the team or from the outside) may not like formatting on save for some reason but they would be forced to either use it or keep enabling and disabling it to not check it into source control. I believe IDE settings are a pretty personal thing and it's easier if everybody just does them for themselves. Enabling format on save for a team member is a 10-second task and we wouldn't need to worry about someone committing their changes, different preferences, etc.

I don't use VSCode myself most of the time so I don't have that strong of an opinion but I believe it's a good practice to keep specific IDEs out of the source.

[mnaamani]

As I was initially hesitant to add this in the first place and your last argument, decided to revert it ad88e65

[mnaamani]

Happy to report the issue with eslint plugin in VSCode looking for tsconfig.json at the root is resolved by adding the following in the workspace settings.json:

  "eslint.workingDirectories": [
    "./cli",
    "./pioneer",
    "./tests/network-tests",
    "./types"
  ]

as advised by @Gamaranto

Is this a legit reason to keep .vscode/settings.json in the repo?

Alternative would be to just have a script that injects these settings into the workspace.

[kdembler]

Shouldn't these be the dependencies of @joystream/eslint-config package instead?

[kdembler]

I'm still little lost in package management in monorepo with workspaces so please let me know if I'm wrong

[mnaamani]

Yes I think that would be neater, will try it out

[kdembler]

For this to work we should add standard above as well

[mnaamani]

Did you find the best placement for it in the array. Shall I make it the last one before prettier ?

[mnaamani]

For context.. this is to introduce the eqeqeq rule

[kdembler]

The example Prettier config has it at the very top

[kdembler]

Seems we should add standard here as well

[kdembler]

This list lacks standard and its peer dependencies, full list: eslint-config-standard eslint-plugin-standard eslint-plugin-promise eslint-plugin-import eslint-plugin-node

[mnaamani]

noted

[mnaamani]

I've been testing eslint plugin in VSCode, with the current setup it seems to be working well but with a couple of issues:

1 - There is still this issue with the plugin complaining about a missing tsconfig.json at the root (there is a an empty file now just to make the plugin actually work) - Francesco pointed at a possible solution here microsoft/vscode-eslint#455

2 - I'm not 100% sure the rules the plugin is applying in the IDE are identical to when eslint is run on the commandline becase I had to add 'no-console': 'off' explicitly for the rule to work in VSCode, but it wasn't required on the commandline. Hopefully other rules are applied consistently otherwise we are gonna have a frustrating experience!

I tried the eslint plugin in Atom but unfortunately it doesn't support .eslintrc.js it only looks for older .eslintrc which is a yaml file which sucks. We don't want to loose benefits of the .js file format so unless anyone is adamant about using Atom over VSCode will not try to fix.

[kdembler]

Just noticed, in example config by Prettier this is actually above all other prettier rules (e.g. prettier/react, etc.)

[kdembler]

The docs say this line:

• Enables eslint-plugin-prettier.
• Sets the prettier/prettier rule to "error".
• Extends the eslint-config-prettier configuration.

So it may be a replacement for prettier above

[mnaamani]

Yes I saw the example. I was basing the ordering on this article

The way I understand it is that eslint-plugin-prettier actually runs prettier itself and reports back the formatting errors based on your prettierrc config. So it seems natural to put it last. Perhaps the order of the plugin itself is not important because its not actually modifying any rules just executing and that happens after all the rules have been configured. I did test the behavior on both the command line and in the IDE and I got the expected results. But I guess it makes sense to follow the maintainers documentation :)

[mnaamani]

I asked out of curiosity prettier/eslint-plugin-prettier#314

[mnaamani]

Okay so I got an answer which makes sense as far as rules go, but didn't satisfy my curiosity about when prettier runs.
So I did a bit of debugging and can confirm that prettier runs (the create() method is called on the plugin) after all the rules in the extends array processed.

[kdembler]

Awesome, good to know

[kdembler]

I think we should also add prettier here, per https://github.com/prettier/eslint-config-prettier#example-configuration

[mnaamani]

I think having 'plugin:prettier/recommended' in the extends array adds it for us, just as your hunch that it also:

So it may be a replacement for prettier above

Based on this code: https://github.com/prettier/eslint-plugin-prettier/blob/master/eslint-plugin-prettier.js#L109

I added it anyway.

[mnaamani]

2 - I'm not 100% sure the rules the plugin is applying in the IDE are identical to when eslint is run on the commandline becase I had to add 'no-console': 'off' explicitly for the rule to work in VSCode, but it wasn't required on the commandline. Hopefully other rules are applied consistently otherwise we are gonna have a frustrating experience!

Somehow after re-testing this, I'm now seeing consistency between the IDE and the commandline linting results.

[kdembler]

LGTM!