AndreaPontrandolfo / sheriff

A comprehensive and opinionated Typescript-first ESLint configuration.
https://www.eslint-config-sheriff.dev
MIT License
104 stars 7 forks source link

[ADD] `eslint-plugin-n` #163

Open lishaduck opened 2 months ago

lishaduck commented 2 months ago

When making #161, I realized both that #162 was needed, and that we don't have any engines checking. You can check engines compatible with transtives via ls-engines --mode=ideal. You can check source compatibility via eslint-plugin-n, which also includes eslint-plugin-es-x. Combined, these tools help make sure that your code supports all versions of node that you say you do. I would assume that we'd add eslint-plugin-n under a node option, and then dogfood it with #162, and just add ls-engines sometime as well. To make adding this non-breaking (though we don't need to avoid majors, of course), I might make sheriff options accept a Partial<SheriffSettings> anyway. Maybe even make it optional at that point. I might PR this change now, because it seems reasonable.

lishaduck commented 2 months ago

To make adding this non-breaking (though we don't need to avoid majors, of course), I might make sheriff options accept a Partial anyway. Maybe even make it optional at that point. I might PR this change now, because it seems reasonable.

Oh, it does that already, just in the SheriffSettings type itself. That should probably be documented though.

AndreaPontrandolfo commented 1 month ago

So, finally getting back to this too.

To make adding this non-breaking (though we don't need to avoid majors, of course), I might make sheriff options accept a Partial anyway

As you already noted, SheriffSettings already has the Partial type.

Maybe even make it optional at that point. I might PR this change now, because it seems reasonable.

Addressed in https://github.com/AndreaPontrandolfo/sheriff/pull/165.

When making https://github.com/AndreaPontrandolfo/sheriff/pull/161, I realized both that https://github.com/AndreaPontrandolfo/sheriff/pull/162 was needed, and that we don't have any engines checking. You can check engines compatible with transtives via ls-engines --mode=ideal. You can check source compatibility via eslint-plugin-n, which also includes eslint-plugin-es-x. Combined, these tools help make sure that your code supports all versions of node that you say you do. I would assume that they'd be located under a node option though.

So, environment checking is a tough one. I avoided it up until now because i'm pretty sure it's gonna skyrocket the complexity of the codebase considerably.

Although, one cool thing that environment checking would enable would be structuredClone for example.

About eslint-plugin-n, what do you like about it? It seems to overlap a lot with eslint-plugin-import and seems like it would be mostly useful in Commonjs environments and only marginally useful in ESM environments.

lishaduck commented 1 month ago

About eslint-plugin-n, what do you like about it?

I've used environment checking with no-unsupported-features/* rules, no-deprecated (though eslint-plugin-deprecated is probably better?), and no-process-exit; I'd assume prefer-promises/* is useful (does unicorn have that one?), process-exit-as-throw is fixing a "bug" that I've hit before (fixed it differently though). There're also some other rules about callbacks, sync apis, and oh, I could see no-unpublished-bin as being useful.

All in all, I think it has merit in ESM environments, although it certainly does overlap with plugin-import.

AndreaPontrandolfo commented 1 month ago

This also kinda overlaps with unicorn/prefer-module. But indeed i like some of the rules. I'm analyzing it.

AndreaPontrandolfo commented 2 weeks ago

no-deprecated (though eslint-plugin-deprecated is probably better?)

Just wanted to point out that @typescript-eslint just added no-deprecated rule to their plugin, which means that we will automatically inherit that rule. So we certainly won't be adopting the eslint-plugin-n version, going forward.