Linting

[goosewobbler]

Enabling linting on a large codebase such as Frame which has never really been linted is always going to be a slow process, especially where we have a mixture of JS and TS and have to rely on a lot of substandard typing to make things work.

With this PR we are enabling linting but not fixing all the issues which arise. The bare config with recommended settings for all plugins produced over 4000 issues. After a fair bit of configuration and some exemptions this is in the low 2000s and fixing all the test issues should get us significantly under 2000. Fixing all the issues in one go is risky -- linting fixes often result in refactoring which comes with its own risks (would like to avoid refactoring or making large changes the closer we get to the omni release) so this PR focusses on the obvious, low-hanging fruit in app / main / resources and everything in test.

Once all issues are handled in some way we can enable a linting check on CI and git pre-push. In the meantime we get the benefit of linting inside VS Code using the Eslint plugin, which can help to influence the development of new code before we arrive at the magical zero issues. It may also help with debugging, I have already identified a few issues which are likely to cause bugs.

Plan for Fixing Issues

This PR mainly deals with issues which arise in the tests (separated out into a few sub-PRs for ease of review)

I expect a large number of issues will be fixed with the signers refactor, additionally a lot of typescript related errors will be fixed when we have types for the store, nebula, etc.

Config

Recommended settings for eslint itself and all plugins below
eslint-config-prettier - configures eslint to play nicely with prettier

We have two overrides sections, one for TS and one for tests. As we move more code over to TS the base config should get smaller.

Plugins

@typescript-eslint/eslint-plugin - flags TS issues, especially around type safety
eslint-plugin-import - flags issues with imports and requires
eslint-plugin-jest - flags antipatterns in our tests
eslint-plugin-jsx-a11y - required as part of the react plugin for effective JSX linting. Includes a lot of accessibility rules which we might want to turn off for now
eslint-plugin-n - community takeover of the hugely popular but abandoned eslint-plugin-node, will eventually move to the eslint-community org. Reports issues with nodeJS usage
eslint-plugin-promise - flags potential misuse of promises and antipatterns around their usage
eslint-plugin-react - flags antipatterns in React usage
eslint-plugin-react-hooks - prevents against misuse of hooks in React
eslint-plugin-testing-library - Helpful tips on how to get the most out of testing-library

Note we are not running eslint-plugin-prettier -- running prettier as an eslint plugin is unperformant and not recommended. Additionally, having separate formatting and linting commands makes for a more stable and debuggable toolchain.

Switching off Rules

Where possible we should switch off rules only as a last resort. Many rules can be configured to work around common usecases which can be considered exceptions.

You can turn off rules for a given file using /* eslint-disable rule1,rule2,rule3 */ at the top of the file - this is better than disabling eslint entirely for a given line or whole file. Any rules turned off in this manner should have an accompanying comment to explain why, and under what circumstances the rules can be re-enabled. If there are no likely near future circumstances under which rules will be re-enabled, the rule(s) should be disabled in the .eslintrc config file.

[mholtzman]

@goosewobbler thanks for this PR -- the write-up is very thorough and informative, and the adoption strategy on the whole makes sense. do you think it makes sense to phase in the linting plugins slowly so that we can more easily see their impact? I propose we enable the base "out of the box" config using recommended rules -- no plugins -- and see where that gets us. we can then discuss which rules, if any, we want to disable or additional rules we want to enable. once that's sorted, we can add plugins one by one in additional PRs to limit the scope of changes we need to worry about in any one PR.

I think another mitigation strategy would be to ignore anything not already written in TS -- when we convert it to TS it will then be beholden to the linting rules and we can bring it in line with our standards at that point.

thoughts?

[goosewobbler]

@mholtzman Definitely a good idea to split this up - I will remove the plugins one by one and provide the numbers of issues resulting and relative effort required in addressing them. Given the work I have already completed on this I think some, not zero, plugins would be good - for instance I think the jest plugin and potentially the import plugin could be used immediately to good effect. I believe many of the issues in the repo are React and a11y-based, which could do with addressing in their own separate PR(s) as I think there will be quite a few a11y rules we will want to turn off.

As for the TS idea, this is also good - however we will always have some JS in the repo and we will want to lint these files too. I will take a look at all of this on Monday, push the other testing fixes I have and we can move this forward.

[goosewobbler]

Closing in favour of #1337