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(eslint-plugin): [init-declarations] refine report locations#8893
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(eslint-plugin): [init-declarations] refine report locations#8893
Uh oh!
There was an error while loading.Please reload this page.
Conversation
Thanks for the PR,@kirkwaiblinger! 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. |
netlifybot commentedApr 10, 2024 • 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 configuration. |
nx-cloudbot commentedApr 10, 2024 • 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.
6b5d7c2 to06bfce3Compare06bfce3 to7ff131dCompareThere 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, sorry for missing this till now! 🚀
It'd be nice to try to get this fixed more cleanly in ESLint core... but my hunch is we don't have a super high chance of that getting accepted.
| ? reportOverride | ||
| : Reflect.get(context, prop, receiver), | ||
| }); | ||
| } |
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.
😬 It's a little awkward having toProxy the base rule. Is there an issue that was ever filed on ESLint about better handling of report locations in this case? I couldn't find one. But even if it's declined as out-of-scope, it'd be good for reference.
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.
I'm not sure... I basically copied this pattern from
typescript-eslint/packages/eslint-plugin/src/rules/lines-around-comment.ts
Lines 417 to 430 in21375f6
| // we can't directly proxy `context` because its `report` property is non-configurable | |
| // and non-writable. So we proxy `customContext` and redirect all | |
| // property access to the original context except for `report` | |
| constproxiedContext=newProxy<typeofcontext>( | |
| customContextastypeofcontext, | |
| { | |
| get(target,path,receiver):unknown{ | |
| if(path!=='report'){ | |
| returnReflect.get(context,path,receiver); | |
| } | |
| returnReflect.get(target,path,receiver); | |
| }, | |
| }, | |
| ); |
and
typescript-eslint/packages/eslint-plugin/src/rules/prefer-destructuring.ts
Lines 196 to 206 in3c6379b
| // we can't directly proxy `context` because its `report` property is non-configurable | |
| // and non-writable. So we proxy `customContext` and redirect all | |
| // property access to the original context except for `report` | |
| returnnewProxy<Context>(customContextastypeofcontext,{ | |
| get(target,path,receiver):unknown{ | |
| if(path!=='report'){ | |
| returnReflect.get(context,path,receiver); | |
| } | |
| returnReflect.get(target,path,receiver); | |
| }, | |
| }); |
And modified it with stylistic preferences.
Following the trail through the git history, it doesn't look like any investigation into alternatives or issues with eslint were documented in those.
(breadcrumbs)
- feat(eslint-plugin): [lines-around-comment] add extension rule #5327: first appearance of proxy pattern, not remarked upon.
- feat(eslint-plugin): add new extended rule
prefer-destructuring#7117: pattern copied, remark infeat(eslint-plugin): add new extended ruleprefer-destructuring#7117 (comment) addresses caching of the proxy, but not the proxy itself.
Will plan to check for eslint issues on the topic before this PR gets merged
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.
before this PR gets merged
Oh@kirkwaiblinger I was going to merge today for the Monday release and consider this a followup. But in that case I'll just let you merge whenever you want.
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.
Followup makes sense to me too! 👍
Happy for us to merge for the release
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.
Openedeslint/eslint#18557
fbff4ce intotypescript-eslint:mainUh oh!
There was an error while loading.Please reload this page.
PR Checklist
Overview
Note that the following example is unchanged