-
-
Notifications
You must be signed in to change notification settings - Fork 2.8k
fix(utils): removed TRuleListener
generic from the createRule
#5036
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
fix(utils): removed TRuleListener
generic from the createRule
#5036
Conversation
Thanks for the PR, @Andarist! typescript-eslint is a 100% community driven project, and we are incredibly grateful that you are contributing to that community. The core maintainers work on this in their personal time, so please understand that it may not be possible for them to review your work immediately. Thanks again! 🙏 Please, if you or your company is finding typescript-eslint valuable, help us sustain the project by sponsoring it transparently on https://opencollective.com/typescript-eslint. As a thank you, your profile/company logo will be added to our main README which receives thousands of unique visitors per day. |
✅ Deploy Preview for typescript-eslint ready!
To edit notification comments on pull requests, go to your Netlify site settings. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM! If this solves your use case then 💯.
I suspect you could probably also remove the TRuleListener
s from CLIEngine.ts
and Rule.ts
, but no worries if that's left for another day.
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## v6 #5036 +/- ##
==========================================
+ Coverage 91.32% 91.36% +0.03%
==========================================
Files 132 364 +232
Lines 1487 12134 +10647
Branches 224 3540 +3316
==========================================
+ Hits 1358 11086 +9728
- Misses 65 748 +683
- Partials 64 300 +236
Flags with carried forward coverage won't be shown. Click here to find out more.
|
NB, #5037 discusses when we should release the next major version. If anybody reading this feels strongly that this change shouldn't have to wait 5 months then please mention there. 🙂 |
…RuleCreateFunction`
Done. I had to leave it in the |
TRuleListener
generic from the createRule
TRuleListener
generic from the createRule
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just re-approving for paperwork purposes 🙂
I believe I'm running into this issue in a monorepo which exports it's internal plugin to a bunch of other packages that have tsconfig project references to the plugin package (and therefore require declaration : true for it). Its 5 months like in 2 days i think. Will this really be merged then? would love to have it asap. (Also let me know if there's another way to acheive the monorepo setup) |
@scamden why do you have tsconfig references to the internal plugin? Plugins really shouldn't export any usable types or API because they are only consumed indirectly via the untyped ESLint config. For a working example we have an internal plugin in this repo that works just fine. |
we depend on the package in package.json as a pnpm workspace dep (so we can use the rules in each package's eslintrc). we also use an automated tool the creates the tsconfig refs based on any pnpm workspace dependencies: https://github.com/Bessonov/set-project-references also we do in fact need the ref i think cause otherwise |
@bradzacher ping on the above. also any word on when this can be merged? |
just another ping on this @bradzacher do you all compile using |
@scamden per our contributing guide, please don't comment on open PRs asking for updates. We're a volunteer team with limited resources or budget. This PR is on the list and we'll get to it when we can. That being said, I started work on making a new v6 version (#5883) and hope to get this in soon, perhaps in the next week or so. |
apologies! and thanks for your work! i was just answering this question
as an anybody who saw a more immediate need. and i now see the "there" that i missed initially as well. my bad. thanks again! |
…nd parser README (typescript-eslint#5864) * docs: Mention wide globs performance implications in monorepos docs and parser readme * Update docs/linting/typed-linting/MONOREPOS.md Co-authored-by: Josh Goldberg <git@joshuakgoldberg.com>
We do use project references and we do use It seems like your workspace trades-off a lighter build without declarations in favour of automated configuration? I definitely understand the desire to use that package to manage project references! It just seems like it's doing the wrong thing by making your production code build "depend on" your dev-only eslint plugins. Generally I'd recommend just making the plugin build a pre-step for your lint command rather than doing the build implicitly via project references. |
if you're using --build that builds referenced projects, if the eslint plugin is a referenced project it has to have Either way, I realized I could solve this by explicitly declaring the return type to be |
BREAKING CHANGE:
Removes generic parameter from public API
PR Checklist
RuleCreator
are not portable #5032Overview
I've dropped
TRuleListener
generic from the public-facing API~. So it no longer should be inferred with types coming from other packages.This generic is still present in the
RuleModule
and in some other things as it seems that it's useful there for extending base rules (which kinda is like an internal thing, from what I understand).