Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[ADD] eslint-plugin-n #163

Open
lishaduck opened this issue Jul 13, 2024 · 5 comments
Open

[ADD] eslint-plugin-n #163

lishaduck opened this issue Jul 13, 2024 · 5 comments
Labels
plugins triaging Issue is still being evaluated. PRs not yet accepted.

Comments

@lishaduck
Copy link
Contributor

lishaduck commented Jul 13, 2024

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
Copy link
Contributor Author

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
Copy link
Owner

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 #165.

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 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
Copy link
Contributor Author

lishaduck commented Jul 29, 2024

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 AndreaPontrandolfo changed the title Add eslint-plugin-n [ADD] eslint-plugin-n Aug 1, 2024
@AndreaPontrandolfo
Copy link
Owner

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

@AndreaPontrandolfo AndreaPontrandolfo added the triaging Issue is still being evaluated. PRs not yet accepted. label Aug 17, 2024
@AndreaPontrandolfo
Copy link
Owner

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
plugins triaging Issue is still being evaluated. PRs not yet accepted.
Projects
None yet
Development

No branches or pull requests

2 participants