search

transloadit / eslint-config-transloadit

Transloadit eslint rules
MIT License
2 stars 1 forks source link

Implement eslint-config-transloadit on main js repos #1

Closed kvz closed 3 years ago

kvz commented 3 years ago

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

Spun off from https://github.com/transloadit/node-sdk/issues/90

TODO:

mifi commented 3 years ago

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

mifi commented 3 years ago

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:

uppy

✖ 9438 problems (9156 errors, 282 warnings)

After eslint --fix:

✖ 1348 problems (1104 errors, 244 warnings)

Complications:

api2

Complications:

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 commented 3 years ago

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

mifi commented 3 years ago

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

https://github.com/transloadit/eslint-config-transloadit/blob/8f345b29f28d6447c77008e187d9d99cac9089e0/index.js#L2

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 commented 3 years ago

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 :thinking:

mifi commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

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

kvz commented 3 years ago

Okay @mifi I have the three repos passing now via eslint-config-transloadit@1.1.1 in:

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

I did create a whole bunch of warns and ignores in the different projects to get there. Summarized the plan here https://github.com/transloadit/uppy/pull/2796#issue-588558040.

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

juliangruber commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

(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 commented 3 years ago

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: https://github.com/transloadit/eslint-config-transloadit/blob/073de1beb6a7e15d764ba985721614f3e8723c87/index.js#L153

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 commented 3 years ago

// perhaps enable in future release?

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

kvz commented 3 years ago

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: https://github.com/transloadit/eslint-config-transloadit/issues/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