Movatterモバイル変換


[0]ホーム

URL:


Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Sign up
Appearance settings

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

Conversation

Andarist
Copy link
Contributor

@AndaristAndarist commentedMay 22, 2022
edited by bradzacher
Loading

BREAKING CHANGE:
Removes generic parameter from public API

PR Checklist

Overview

I've droppedTRuleListener 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 theRuleModule 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).

@nx-cloud
Copy link

nx-cloudbot commentedMay 22, 2022
edited
Loading

☁️ Nx Cloud Report

CI is running/has finished running commands for commitf09d478. As they complete they will appear below. Click to see the status, the terminal output, and the build insights.

📂 See all runs for this branch


✅ Successfully ran 46 targets

Sent with 💌 fromNxCloud.

@typescript-eslint
Copy link
Contributor

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.

@netlify
Copy link

netlifybot commentedMay 22, 2022
edited
Loading

Deploy Preview fortypescript-eslint ready!

NameLink
🔨 Latest commitcc2fc8c
🔍 Latest deploy loghttps://app.netlify.com/sites/typescript-eslint/deploys/628aaf6bb254e6000913f3e2
😎 Deploy Previewhttps://deploy-preview-5036--typescript-eslint.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to yourNetlify site settings.

@JoshuaKGoldbergJoshuaKGoldberg added the breaking changeThis change will require a new major version to be released labelMay 22, 2022
JoshuaKGoldberg
JoshuaKGoldberg previously approved these changesMay 22, 2022
Copy link
Member

@JoshuaKGoldbergJoshuaKGoldberg left a 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 theTRuleListeners fromCLIEngine.ts andRule.ts, but no worries if that's left for another day.

@codecov
Copy link

codecovbot commentedMay 22, 2022
edited
Loading

Codecov Report

Merging#5036 (fcf3f9d) intov6 (1f60fae) willincrease coverage by0.03%.
The diff coverage is96.62%.

❗ Current headfcf3f9d differs from pull request most recent headf09d478. Consider uploading reports for the commitf09d478 to get more accurate results

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
FlagCoverage Δ
unittest91.36% <96.62%> (+0.03%)⬆️

Flags with carried forward coverage won't be shown.Click here to find out more.

Impacted FilesCoverage Δ
packages/eslint-plugin/src/rules/await-thenable.ts100.00% <ø> (ø)
...ages/eslint-plugin/src/rules/ban-tslint-comment.ts100.00% <ø> (ø)
...-plugin/src/rules/explicit-member-accessibility.ts97.43% <ø> (ø)
...plugin/src/rules/explicit-module-boundary-types.ts91.04% <ø> (ø)
...kages/eslint-plugin/src/rules/func-call-spacing.ts96.87% <ø> (ø)
packages/eslint-plugin/src/rules/indent.ts92.85% <ø> (ø)
...kages/eslint-plugin/src/rules/init-declarations.ts100.00% <ø> (ø)
...ackages/eslint-plugin/src/rules/keyword-spacing.ts100.00% <ø> (ø)
...nt-plugin/src/rules/lines-between-class-members.ts92.85% <ø> (ø)
.../eslint-plugin/src/rules/member-delimiter-style.ts94.73% <ø> (ø)
... and308 more

@JoshuaKGoldberg
Copy link
Member

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. 🙂

@Andarist
Copy link
ContributorAuthor

I suspect you could probably also remove the TRuleListeners from CLIEngine.ts and Rule.ts, but no worries if that's left for another day.

Done. I had to leave it in theRuleModule type inRule.ts though (that's the core of why it's needed in the first place).

@bradzacherbradzacher changed the titlerefactor(utils)!: removedTRuleListener generic from thecreateRulefix(utils): removedTRuleListener generic from thecreateRuleMay 23, 2022
Copy link
Member

@JoshuaKGoldbergJoshuaKGoldberg left a 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
Copy link

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. 🙂

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)

@bradzacher
Copy link
Member

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

scamden commentedOct 20, 2022
edited
Loading

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 otherwisetsc --build won't know to rebuild the eslint-plugin package from those that depend on it to add to their eslintrc's, plz tell me i'm wrong :)

@scamden
Copy link

@bradzacher ping on the above. also any word on when this can be merged?

@scamden
Copy link

just another ping on this@bradzacher do you all compile usingtsc --build option? i believe references are necessary for this to function

@JoshuaKGoldberg
Copy link
Member

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

apologies! and thanks for your work! i was just answering this question

If anybody reading this feels strongly that this change shouldn't have to wait 5 months then please mention there. 🙂

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!

JoshuaKGoldberg reacted with heart emoji

adnanhashmi09and others added2 commitsOctober 25, 2022 19:35
…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>
@JoshuaKGoldbergJoshuaKGoldberg merged commit0414e4d intotypescript-eslint:v6Oct 25, 2022
@bradzacher
Copy link
Member

@scamden

We do use project references and we do use--build, but we don't have any packages that depend on our plugins, so there is no need for the plugins to be compiled with declarations.

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

We do use project references and we do use--build, but we don't have any packages that depend on our plugins, so there is no need for the plugins to be compiled with declarations.

if you're using --build that builds referenced projects, if the eslint plugin is a referenced project it has to havecomposite : true which auto-enables declarations. if it's not a referenced project it it won't get built by --build, so maybe you are building it independently? point is if we want to use --build to auto build the things we have to build declarations whether dependents need them or not.

Either way, I realized I could solve this by explicitly declaring the return type to beReturnType<typeof createRule> so we are unblocked. Thanks for the attention and for all the excellent work on this plugin!

@github-actionsgithub-actionsbot locked asresolvedand limited conversation to collaboratorsNov 17, 2022
Sign up for freeto subscribe to this conversation on GitHub. Already have an account?Sign in.
Reviewers

@JoshuaKGoldbergJoshuaKGoldbergJoshuaKGoldberg approved these changes

Assignees
No one assigned
Labels
breaking changeThis change will require a new major version to be released
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Bug: Created rules usingRuleCreator are not portable
5 participants
@Andarist@JoshuaKGoldberg@scamden@bradzacher@adnanhashmi09

[8]ページ先頭

©2009-2025 Movatter.jp