fix(eslint-plugin): [init-declarations] refine report locations #8893

kirkwaiblinger · Apr 10, 2024

PR Checklist

Addresses an existing open issue: fixes (partially) Enhancement: Improve lint error report location #8543
That issue was marked as accepting prs
Steps in Contributing were taken

Overview

/* eslint @typescript-eslint/init-declarations: ["error", "always"] */
// before
let bar: number | undefined | string;
    ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
// after
let bar: number | undefined | string;
    ^^^

Note that the following example is unchanged

/* eslint @typescript-eslint/init-declarations: ["error", "never"] */
let bar: number | undefined | string = 3;
    ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

typescript-eslint · Apr 10, 2024

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 on https://opencollective.com/typescript-eslint.

netlify · Apr 10, 2024

✅ Deploy Preview for typescript-eslint ready!

Name	Link
🔨 Latest commit	`769eddc`
🔍 Latest deploy log	https://app.netlify.com/sites/typescript-eslint/deploys/665fb9afbd2b2000080e6f18
😎 Deploy Preview	https://deploy-preview-8893--typescript-eslint.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.
Lighthouse	1 paths audited Performance: 95 (🔴 down 4 from production) Accessibility: 100 (no change from production) Best Practices: 92 (no change from production) SEO: 90 (🔴 down 8 from production) PWA: 80 (no change from production) View the detailed breakdown and full score reports

To edit notification comments on pull requests, go to your Netlify site configuration.

nx-cloud · Apr 10, 2024

☁️ Nx Cloud Report

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

📂 See all runs for this CI Pipeline Execution

✅ Successfully ran 31 targets

Sent with 💌 from NxCloud.

JoshuaKGoldberg

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.

JoshuaKGoldberg · Jun 2, 2024

packages/eslint-plugin/src/rules/init-declarations.ts

+            ? reportOverride
+            : Reflect.get(context, prop, receiver),
+      });
+    }


😬 It's a little awkward having to Proxy 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.

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 in 21375f6

// 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`

const proxiedContext = new Proxy<typeof context>(

customContext as typeof context,

{

get(target, path, receiver): unknown {

if (path !== 'report') {

return Reflect.get(context, path, receiver);

}

return Reflect.get(target, path, receiver);

},

},

);

and

typescript-eslint/packages/eslint-plugin/src/rules/prefer-destructuring.ts

Lines 196 to 206 in 3c6379b

// 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`

return new Proxy<Context>(customContext as typeof context, {

get(target, path, receiver): unknown {

if (path !== 'report') {

return Reflect.get(context, path, receiver);

}

return Reflect.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 in feat(eslint-plugin): add new extended rule prefer-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

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.

Followup makes sense to me too! 👍
Happy for us to merge for the release

Opened eslint/eslint#18557

kirkwaiblinger changed the title ~~fix(eslint-plugin): [init-declarations] refine report loc~~ fix(eslint-plugin): [init-declarations] refine report locations Apr 10, 2024

yeonjuan mentioned this pull request Apr 10, 2024

Enhancement: Improve lint error report location #8543

Closed

9 tasks

kirkwaiblinger force-pushed the init-declarations-refine-report-loc branch from 6b5d7c2 to 06bfce3 Compare April 10, 2024 07:25

[init-declarations] refine report loc

7ff131d

kirkwaiblinger force-pushed the init-declarations-refine-report-loc branch from 06bfce3 to 7ff131d Compare April 10, 2024 07:26

kirkwaiblinger marked this pull request as ready for review April 10, 2024 07:42

Merge branch 'main' into init-declarations-refine-report-loc

b0b686d

kirkwaiblinger added the enhancement New feature or request label May 4, 2024

JoshuaKGoldberg approved these changes Jun 2, 2024

View reviewed changes

JoshuaKGoldberg added the 1 approval >=1 team member has approved this PR; we're now leaving it open for more reviews before we merge label Jun 2, 2024

JoshuaKGoldberg assigned kirkwaiblinger Jun 3, 2024

Merge branch 'main' into init-declarations-refine-report-loc

769eddc

kirkwaiblinger merged commit fbff4ce into typescript-eslint:main Jun 5, 2024
61 checks passed

kirkwaiblinger deleted the init-declarations-refine-report-loc branch June 5, 2024 02:09

github-actions bot locked as resolved and limited conversation to collaborators Jun 15, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Uh oh!

fix(eslint-plugin): [init-declarations] refine report locations #8893

fix(eslint-plugin): [init-declarations] refine report locations #8893

Uh oh!

kirkwaiblinger commented Apr 10, 2024

Uh oh!

typescript-eslint bot commented Apr 10, 2024

Uh oh!

netlify bot commented Apr 10, 2024 •

edited

Loading

Uh oh!

nx-cloud bot commented Apr 10, 2024 •

edited

Loading

Uh oh!

JoshuaKGoldberg left a comment

Uh oh!

JoshuaKGoldberg Jun 2, 2024

Uh oh!

kirkwaiblinger Jun 3, 2024

Uh oh!

JoshuaKGoldberg Jun 3, 2024

Uh oh!

kirkwaiblinger Jun 3, 2024

Uh oh!

kirkwaiblinger Jun 5, 2024

Uh oh!

Uh oh!

Uh oh!

	// 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`
	const proxiedContext = new Proxy<typeof context>(
	customContext as typeof context,
	{
	get(target, path, receiver): unknown {
	if (path !== 'report') {
	return Reflect.get(context, path, receiver);
	}
	return Reflect.get(target, path, receiver);
	},
	},
	);

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

Uh oh!

fix(eslint-plugin): [init-declarations] refine report locations #8893

fix(eslint-plugin): [init-declarations] refine report locations #8893

Uh oh!

Conversation

kirkwaiblinger commented Apr 10, 2024

PR Checklist

Overview

Uh oh!

typescript-eslint bot commented Apr 10, 2024

Uh oh!

netlify bot commented Apr 10, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

✅ Deploy Preview for typescript-eslint ready!

Uh oh!

nx-cloud bot commented Apr 10, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

☁️ Nx Cloud Report

Uh oh!

JoshuaKGoldberg left a comment

Choose a reason for hiding this comment

Uh oh!

JoshuaKGoldberg Jun 2, 2024

Choose a reason for hiding this comment

Uh oh!

kirkwaiblinger Jun 3, 2024

Choose a reason for hiding this comment

Uh oh!

JoshuaKGoldberg Jun 3, 2024

Choose a reason for hiding this comment

Uh oh!

kirkwaiblinger Jun 3, 2024

Choose a reason for hiding this comment

Uh oh!

kirkwaiblinger Jun 5, 2024

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

netlify bot commented Apr 10, 2024 •

edited

Loading

nx-cloud bot commented Apr 10, 2024 •

edited

Loading