ESLint

[lishaduck]

This is very much a WIP. I'm working on modernizing the linting configuration now that node 10&12 are dropped.

I have a few stylistic questions, but first I want to get tests passing.

[lishaduck]

Oh, tests pass now 🤷‍♂️ Whatever jest. 😆

And they don't again, after a force-push with 0 changes. 🤣

[socket-security[bot]]

New and removed dependencies detected. Learn more about Socket for GitHub ↗︎

Package	New capabilities	Transitives	Size	Publisher
npm/@eslint-community/eslint-plugin-eslint-comments@4.3.0	None	+2	97.9 kB	eslint-community-bot
npm/@typescript-eslint/eslint-plugin@6.21.0	Transitive: environment, filesystem	+40	7.24 MB	jameshenry
npm/@typescript-eslint/parser@6.21.0	Transitive: environment, filesystem	+32	2.55 MB	jameshenry
npm/eslint-plugin-n@16.6.2	filesystem Transitive: environment, unsafe	+20	2.17 MB	weiran.zsd
npm/eslint-plugin-promise@6.2.0	None	0	69.6 kB	eslint-community-bot
npm/eslint-plugin-security@2.1.1	None	+2	468 kB	eslint-community-bot

🚮 Removed packages: npm/@typescript-eslint/eslint-plugin@5.62.0, npm/@typescript-eslint/parser@5.62.0, npm/eslint-plugin-node@11.1.0

View full report↗︎

[lishaduck]

May I request a re-run of CI? It seems like is a flake. It passed a moment ago, and the force-push shows it's clearly exactly the same code, just based off main so I could read my git client.

Nah, I'll just push a new commit.

[lishaduck]

Also, this is going to conflict with #147. Do you want me to see how much I can port from that PR?

[jfmengels]

@lishaduck That sounds good to me. I'm not sure what to do about that PR at the moment (I'm also not sure as to the state I left it in to be honest). But feel free to postpone that porting to after this gets merged, we can sort the conflicts later :man_shrugging:

[lishaduck]

Ok! ESLint v9 doesn't support Node 16, so we're still on 8. ESLint plugins have followed suit, so we're lagging a few versions behind XO, n, and Unicorn. I also didn't add JSDoc b/c @import is only supported in node 18+, so it warns (and you can't comment in jsdocs). Otherwise, I think it's all up to date.

PS: I know you don't like expiring todos (and I agree with your logic!), but I think these are fine, given how they work.

[lishaduck]

This should be good to go! Nope. Nonetheless:

Follow ups:

• Reland the actual stuff from #155
• Get #147 merged in some form
• Enable prefer-await-to-then?
• Add eslint-plugin-perfectionist?
• Other rules that the XO config doesn't include b/c peerdeps were funny.
• Bump Prettier
• Add knip

Also:

• npm dedupe
• Improve GHA (security, up-to-date, and matrix)
• Cache TS and ESLint

[lishaduck]

Moving this back to WIP. Those flakes were updated snapshots, now I have to go back through everything. Also, I realized that the XO cli does a lot more than the config, so I'll fun with that :) Do you mind if I make a TODO list issue? Both so I remember what needs to get done, but also so you can see what I'm working on?

[jfmengels]

Moving this back to WIP.

Sure. Let's focus on getting it merged again if that's alright with you (and do unrelated work to that in a separate PR).

Do you mind if I make a TODO list issue?

Sure, go ahead.

Enable prefer-await-to-then?

I'd say not, I find that .then does provide some purpose even in the presence of await. For instance, with the rule you can't write this

promises = await Promise.all([
    someFunction(data).then(doSomething),
    someOtherPromise,
    ...
]);

which can be useful if you want to do more things in parallel. Or just a = await someFunction(data).then(doSomething) to avoid intermediate variable declarations.

I realized that the XO cli does a lot more than the config, so I'll fun with that :)

Yes, it does a bit more, though I don't remember what it did. To be honest, I don't think we need to add too many ESLint rules and other tools (but feel free to suggest them!), I mostly want to help avoid regressions, and get some more guarantees here and there so that this project is maintainable and bug-free.

So far (surprisingly), the migration to TS has not caught any noticeable bugs, so the investment has been quite poor. Not that we shouldn't continue (especially if it's a bit fun for some people :smile:). The XO rules can definitely wait I'd say (although, I'm not sure what has now been disabled).

[lishaduck]

Moving this back to WIP.

Sure. Let's focus on getting it merged again if that's alright with you (and do unrelated work to that in a separate PR).

Yeah. I just want to fix tests and the lints I realized weren't getting applied right. That other comment was just a general todo list. ↓

Do you mind if I make a TODO list issue?

Sure, go ahead.

👍

Enable prefer-await-to-then?

I'd say not, I find that .then does provide some purpose even in the presence of await. For instance, with the rule you can't write this

promises = await Promise.all([
    someFunction(data).then(doSomething),
    someOtherPromise,
    ...
]);

which can be useful if you want to do more things in parallel. Or just a = await someFunction(data).then(doSomething) to avoid intermediate variable declarations.

Yeah. I tried turning it on but ran into that, so I wasn't sure.

I realized that the XO cli does a lot more than the config, so I'll fun with that :)

Yes, it does a bit more, though I don't remember what it did. To be honest, I don't think we need to add too many ESLint rules and other tools (but feel free to suggest them!), I mostly want to help avoid regressions, and get some more guarantees here and there so that this project is maintainable and bug-free.

Essentially, it applies the plugins, so migrating to the config turned off a lot of important rules, like eslint-plugin-n (with eslint-plugin-es-x), for example.

So far (surprisingly), the migration to TS has not caught any noticeable bugs, so the investment has been quite poor. Not that we shouldn't continue (especially if it's a bit fun for some people 😄).

It's a lot of fun :) Once this lot is finished, I plan on finished ts-ifying the rest. It's certainly helpful to the extent of maintenance, as TSEslint 6 caught a good deal. It's still local for now until I finish unicorn and n, but it's definitely a good investment. And I turned off the abusive promises rules! Lots of stuff to fix (though mostly modernization and theoretical data races, not actual bugs).

The XO rules can definitely wait I'd say (although, I'm not sure what has now been disabled).

Unicorn was off, but I've almost finished reenabling it. N needs to get on. We don't use AVA, so that can remain off.

[lishaduck]

Ooooh. Unicorn found a unicode bug! Probably not a real issue, but String.prototype.split() breaks apart surrogate pairs.

[lishaduck]

🤞 I think I'm done.

P.S: Sorry for the diff 🙃 I just read it all. Took me a good 5 minutes, and I was just skimming. Not fun. At least it was mostly automated.

[jfmengels]

Thank you for all the changes @lishaduck. I hope it will help make the project more maintainable :relaxed:

I'm happy to merge as-is (or rather "rebase fast-forward" the branch manually outside of GitHub, to avoid you needing to rebase the other PRs) and delay the remaining feedback for another PR, would you prefer that?

[lishaduck]

Thank you for all the changes @lishaduck. I hope it will help make the project more maintainable ☺️

I'm happy to merge as-is (or rather "rebase fast-forward" the branch manually outside of GitHub, to avoid you needing to rebase the other PRs) and delay the remaining feedback for another PR, would you prefer that?

Eh, I don't mind rebasing, but if you'd prefer that I don't really care.

[jfmengels]

I'll let you make the remaining changes then :+1: