Implement eslint-config-transloadit on main js repos

[kvz]

It would be great to roll out a new linting standard across our main js codebases.

Spun off from transloadit/node-sdk#90

TODO:

• Apply to node-sdk as well
• See how many more rules in API we can easily add back
• Test fix-crm
• Attend to feedback in https://github.com/transloadit/transloadit-api2/pull/2287
• Attend to feedback in Update Linter uppy#2796
• Attend to feedback in Transloadit Linter Upgrade node-sdk#100
• Attend to feedback in Transloadit Linter kvz/lanyon#62
• Attend to feedback in https://github.com/transloadit/content/pull/1443
• Attend to feedback in https://github.com/transloadit/botty/pull/26
• Merge Update Linter uppy#2796
• Merge Transloadit Linter Upgrade node-sdk#100
• Merge Transloadit Linter kvz/lanyon#62
• Merge https://github.com/transloadit/content/pull/1443
• Merge https://github.com/transloadit/botty/pull/26
• Attend to feedback in Update Linter tus/tus-js-client#249
• Merge Update Linter tus/tus-js-client#249
• Fix CI for https://github.com/transloadit/transloadit-api2/pull/2287/files (https://ci.transloadit.com/job/api2-bionic-masteronly-test/1243/console vs https://ci.transloadit.com/job/api2-bionic-playground2-test/143/console) (ergo, one failing test that also fails in master right now)
• Test on staging (https://ci.transloadit.com/job/api2-bionic-playground2-build/20182/console)
• Merge https://github.com/transloadit/transloadit-api2/pull/2287

[mifi]

Ok I'll try that. I added a PR for node-sdk so you can see which things changed there: transloadit/node-sdk#95

[mifi]

I tried it on the different repos and here are the results:

content

✖ 2964 problems (2610 errors, 354 warnings)

After eslint --fix:

✖ 1624 problems (1436 errors, 188 warnings)

Complications:

• React
• Complex eslint config ("config": "./.lanyon/webpack.config.js" etc)

uppy

✖ 9438 problems (9156 errors, 282 warnings)

After eslint --fix:

✖ 1348 problems (1104 errors, 244 warnings)

Complications:

• lint-staged
• React
• Complex eslint config

api2

Complications:

• some Typescript

api2 core

✖ 193 problems (171 errors, 22 warnings)

After eslint --fix:

✖ 149 problems (127 errors, 22 warnings)

api2/api2

✖ 18423 problems (14728 errors, 3695 warnings)

After eslint --fix:

✖ 14411 problems (10709 errors, 3702 warnings)

api2/crm

✖ 185 problems (144 errors, 41 warnings)

After eslint --fix:

✖ 112 problems (87 errors, 25 warnings)

api2/statuspage

✖ 547 problems (491 errors, 56 warnings)

After eslint --fix:

✖ 387 problems (357 errors, 30 warnings)

So as you can see there are quite a few issues that need to be solved for this. I think api2 is the easiest to include the new eslint config for, because it has the least errors and it is Node.js based (no react, webpack and not much magic in the eslint configuration).

As for the others, we could always try to make it work by fixing the config issues and disabling failing rules until it works, but it will take quite some time (I reckon days), so it may not be worth the big time investment.

[kvz]

Question, how does AirBnB solve React? I think an official Transloadit lint kit should support this out of the box if possible

[mifi]

Yes, we can extend airbnb instead of airbnb-base here:

eslint-config-transloadit/index.js

Line 2 in 8f345b2

extends : 'airbnb-base',

Airbnb solved it by creating two packages. their airbnb extends airbnb-base, and adds react related rules.

We can have two packages or just one. The react one should work fine for pure Node projects without react code too, but then need to install more peer-dependencies, e.g. npm install --save-dev eslint-plugin-jsx-a11y@^#.#.# eslint-plugin-import@^#.#.# eslint-plugin-react@^#.#.# eslint-plugin-react-hooks@^#.#.#)

[kvz]

We can't bundle those as dependencies to our own module? Would that just mean the api carries some extra unneeded modules? Because I think i'd be okay with that if they were devDependencies 🤔

[mifi]

We can't bundle those as dependencies to our own module?

Sure, we can do that also. I'm not 100% sure why airbnb has them as peerDependencies, but I would think that it gives flexibility as they can specify a version range in peerDependencies, and then the consuming repos can choose any version of those plugins they may need.

Would that just mean the api carries some extra unneeded modules? Because I think i'd be okay with that if they were devDependencies 🤔

You're right, then it would carry some unneeded modules (devDependencies only)

[kvz]

I think AirBnB probably doesn't want to impose the extra weight on anyone, since it's used by such a large community they could get pushback on that and hurt adoption in back-end projects. But for us, for internal use, i think it is fine and makes for a better DX. Just install one module and ~done. Have the exact same linting experience across projects

[mifi]

Yes. I can try again with airbnb instead of airbnb-base instead tomorrow. But I think there will still be a lot of tweaks needed to get the error count down.

[kvz]

Okay. Let's start with the api and build an ignore list so that we have a pass. Then remove ignores one by one, those could be separate PRs. Otherwise it's going to be too big of a bang, and we'll have a PR open for weeks, only to accumulate so many merge conflicts that we'll abandon it :)

What do you think? If it still catches the things we are already catching, we'll at least not regress, and can progress one rule at a time?

Looking at the ignore list will also give us a better idea if some rules are better ignored in the lint module or no

[mifi]

I created an enormous PR for api2 now with the results of eslint --fix

[kvz]

Okay @mifi I have the three repos passing now via [email protected] in:

https://github.com/transloadit/transloadit-api2/pull/2287
https://github.com/transloadit/content/pull/1443
transloadit/uppy#2796

I did create a whole bunch of warns and ignores in the different projects to get there. Summarized the plan here transloadit/uppy#2796 (comment).

What do you think? /cc @juliangruber @goto-bus-stop

[juliangruber]

I've reviewed the one in api2, I assume comments there also apply to the other repos. In general, I think we're on a good path and that this will be very useful 👍 I do hope we can turn on the extraneous dependencies rule.

[mifi]

Great job on the eslint'ing @kvz !

Possible improvements:

The rules that now have been disabled inside https://github.com/transloadit/eslint-config-transloadit/blob/main/index.js because one project requires too much work to abide to the rules, but we want to enable the rules in the future, I think it could be better to move those rules into the respective projects, and not have them in our central eslint config. If they are in our central eslint config, then other new/smaller projects cannot benefit from those rules because they are now globally disabled.

Also, should we create one issue for each rule that we believe should be re-enabled in the future, but have been disabled due to too much work abiding to them? Then the issues could act as a todos.

[kvz]

The rules that now have been disabled inside https://github.com/transloadit/eslint-config-transloadit/blob/main/index.js because one project requires too much work to abide to the rules, but we want to enable the rules in the future, I think it could be better to move those rules into the respective projects, and not have them in our central eslint config. If they are in our central eslint config, then other new/smaller projects cannot benefit from those rules because they are now globally disabled.

Do you have specific ones in mind? The rules that i disabled there are rules that all projects don't want/need, and then per project we track a large batch that we want enabled in good time in their own .eslintrc. So i think we're on the same page although maybe not regarding which specific rules qualify for what.

Also, should we create one issue for each rule that we believe should be re-enabled in the future, but have been disabled due to too much work abiding to them? Then the issues could act as a todos.

I think that is fairly noisy filling up our board. Since all the rules we disabled are warnings in each project, and those show up in editors and prs, the local .eslintrc serves as a todo list already maybe?

One thing i would like a hand with is peerDependencies. I'm not sure which ones that i hadded as local deps to this project should actually be moved too peerdeps, i think quite a few but i think you're more proficient at that topic.

[kvz]

(and then maybe the readme instructions can also contain a one-liner to install the peerdeps, like yarn add $(npm get peer deps etc)

[mifi]

Oh ok. Then we’re on the same page. I just looked through quickly and i noticed some rules that it seemed like we wanted to enable in the future like this:

eslint-config-transloadit/index.js

Line 153 in 073de1b

// perhaps enable in future release?

i’ll have a look thru the rules and see if there are some disabled ones that I see great benefit from and report back.

i agree one issue per rule is too much.

[kvz]

// perhaps enable in future release?

ah that needs some clarification yes, let's revisit once you are done with your evaluation

[kvz]

Last todo done: https://github.com/transloadit/transloadit-api2/pull/2287 was merged into master (after CI & staging agreed). This is being built now as: https://ci.transloadit.com/job/api2-bionic-masteronly-build/1435/console (current stable 1434).

If we reach consensus on a rule that needs changing, we can add them here: #3

So long as v2 wasn't pushed out, we can temporarily change .eslintrc in the local repo to turn a warning off for such a rule.

For all pending PRs that will have huge merge conflicts, something like this should make those less painful https://gist.github.com/kvz/b205470849f0489549f43c7a27a406dd