Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork2.8k
fix(utils): removedTRuleListener
generic from thecreateRule
#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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
fix(utils): removedTRuleListener
generic from thecreateRule
#5036
Uh oh!
There was an error while loading.Please reload this page.
Conversation
nx-cloudbot commentedMay 22, 2022 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
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 onhttps://opencollective.com/typescript-eslint. As a thank you, your profile/company logo will be added to our main README which receives thousands of unique visitorsper day. |
netlifybot commentedMay 22, 2022 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
✅ Deploy Preview fortypescript-eslint ready!
To edit notification comments on pull requests, go to yourNetlify 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 theTRuleListener
s fromCLIEngine.ts
andRule.ts
, but no worries if that's left for another day.
codecovbot commentedMay 22, 2022 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
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 thecreateRule
TRuleListener
generic from thecreateRule
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 🙂
scamden commentedOct 20, 2022
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. |
scamden commentedOct 20, 2022 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
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 |
scamden commentedOct 21, 2022
@bradzacher ping on the above. also any word on when this can be merged? |
scamden commentedOct 25, 2022
just another ping on this@bradzacher do you all compile using |
@scamden perour 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. |
scamden commentedOct 25, 2022
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.mdCo-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. |
scamden commentedOct 31, 2022
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 |
Uh oh!
There was an error while loading.Please reload this page.
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).