Scripts: Ensure that typescript-eslint checks for unused vars #62925

Chrico · 2024-06-27T13:03:30Z

What?

'no-unused-vars': 'off' in eslint-plugin/configs/recommanded.js will also disable the Typescript eslint when running wp-scripts lint-js.

Why?

eslint/no-unused-vars got disabled when introducing support for TypeScript with #27143 (2-3 years ago) when Gutenberg was moving to Typescript. By disabling this feature, the Typescript eslint will not be executed. We actively enable "no-unused-vars" and "@typescript-eslint/no-unused-vars" and additionally setting "ignoreRestSiblings" to "true", which is actively used in Gutenberg.

See more information in: #54305

Testing Instructions

Create a new file:

index.ts

class Foo {
	protected bar: string;
	public constructor( protected value: string ) {
		this.bar = 'bar';
	}

	public getValue = (): string => {
		return this.value;
	};
}
new Foo( 1 );

with package.json:

{
    "name": "tsc-eslint",
    "version": "1.0.0",
    "license": "GPL-2.0-or-later",
    "devDependencies": {
        "@wordpress/scripts": "^25.0.0",
        "typescript": "^4.9.4"
    }
}

and execute on cli:

$ wp-scripts lint-js *.{ts,tsx}

before this fix the Typescript eslint did not catch any type errors:

Done in 1.83s.

after this fix the Typescript eslint will match errors:

/{snip}/index.ts
  5:32  error  'value' is defined but never used  no-unused-vars

✖ 1 problem (1 error, 0 warnings)

error Command failed with exit code 1.
info Visit https://yarnpkg.com/en/docs/cli/run for documentation about this command.

…hecker is executed. eslint/no-unused-vars got disabled when introducing support for TypeScript with WordPress#27143 (2-3 years ago) when Gutenberg was moving to Typescript. By disabling this feature, the Typescript type checker will not be executed. We actively enable "no-unused-vars" and "@typescript-eslint/no-unused-vars" and additionally setting "ignoreRestSiblings" to "true", which is actively used in Gutenberg. See more information in: WordPress#54305

github-actions · 2024-06-27T13:03:45Z

The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the props-bot label.

If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message.

Co-authored-by: Chrico <chrico@git.wordpress.org>
Co-authored-by: sirreal <jonsurrell@git.wordpress.org>
Co-authored-by: gziolo <gziolo@git.wordpress.org>

To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook.

github-actions · 2024-06-27T13:03:52Z

👋 Thanks for your first Pull Request and for helping build the future of Gutenberg and WordPress, @Chrico! In case you missed it, we'd love to have you join us in our Slack community.

If you want to learn more about WordPress development in general, check out the Core Handbook full of helpful information.

gziolo · 2024-06-28T06:31:57Z

I see that changes work as CI reports many linting errors that would have to be fixed:

The question is whether both rules should be enabled at the same time, as they often report the same issues. I don't know the details but at least for the Gutenberg repository no-unused-var is reporting violations way often.

Chrico · 2024-06-28T07:18:25Z

@gziolo
The @typescript-eslint/no-unused-vars extends the eslint/no-unused-vars. I guess we need to change it to following:

{
   'no-unused-vars': 'off',
   '@typescript-eslint/no-unused-vars': [ 'error', { ignoreRestSiblings: true } ]
}

At least, this is the recommended way in the docs https://typescript-eslint.io/rules/no-unused-vars/#how-to-use ;-)

…pt-eslint/no-unused-vars"

gziolo · 2024-06-28T09:08:06Z

There are still 24 errors reported, way better but it probably reports unused variables that need to be put in the function callbacks:

Chrico · 2024-06-28T09:19:24Z

Should we ( I ?) also fix the problems which now occur or is this a "follow-up" Issue/PR? :-) Probably, depending on how we want to continue, some code might needs to be touched and/or we need to add some // eslint-disable-next-line @typescript-eslint/no-unused-vars ?!

gziolo · 2024-06-28T09:23:30Z

It needs to be evaluated before landing whether these issues are correctly highlighted and the should get fixed. Alternatively, maybe there are some config options that could relax these checks to pass the validation.

Chrico · 2024-06-28T09:37:11Z

The new reported errors look really like a quick win. Maybe just needs some jep/nope from you (?). :-)

Quickly went through all items:

https://github.com/WordPress/gutenberg/blob/v18.6.1/packages/components/src/autocomplete/index.tsx#L16
--> Could be removed, it is not used in that file.

https://github.com/WordPress/gutenberg/blob/v18.6.1/packages/components/src/border-box-control/border-box-control-visualizer/component.tsx#L4
--> Could be removed, it is not used in that file.

https://github.com/WordPress/gutenberg/blob/v18.6.1/packages/components/src/date-time/date-time/index.tsx#L10
--> Could be removed, it is not used in that file.

https://github.com/WordPress/gutenberg/blob/v18.6.1/packages/components/src/font-size-picker/utils.ts#L4
--> Could be removed, it is not used in that file.

https://github.com/WordPress/gutenberg/blob/v18.6.1/packages/components/src/palette-edit/test/index.tsx#L19

_ is unused, probably could be rewritten into:

[ ...Array( input.value.length ) ].forEach( async () => await press.Backspace() )

https://github.com/WordPress/gutenberg/blob/v18.6.1/packages/components/src/private-apis.ts#L4
--> Could be removed, it is not used in that file.

https://github.com/WordPress/gutenberg/blob/v18.6.1/packages/components/src/theme/stories/index.story.tsx#L64
--> disable for that line?

https://github.com/WordPress/gutenberg/blob/v18.6.1/packages/components/src/toggle-group-control/toggle-group-control/component.tsx#L12
--> Could be removed, it is not used in that file.

https://github.com/WordPress/gutenberg/blob/v18.6.1/packages/compose/src/higher-order/test/compose.ts#L33
https://github.com/WordPress/gutenberg/blob/v18.6.1/packages/compose/src/higher-order/test/pipe.ts#L33

--> Seems like the case f is really not used in the following expect(). So: Remove that line or add to the expect()?

gziolo · 2024-06-28T10:19:34Z

Maybe you could commit to all the proposed changes to help decide.

token-list // disable and enable eslint "no-unused-var" for supports()-method.

Chrico · 2024-06-28T13:46:53Z

Seems like, every time something is fixed, something new appears. The wp-scripts lint-js does not show all errors at once (probably due the hugh amount of warnings). This might can take a bit longer, but probably also needs some help to not break things ;-)

gziolo · 2024-06-28T14:43:32Z

So far so good. 12 errors fixed, 12 more to go.

Chrico · 2024-06-28T15:29:39Z

Here we go @gziolo :) ☕

sirreal

Thanks for working on this, good spot! I've left a few suggestions I'd like to see changed before this moves ahead.

Will you also add a Breaking Changes changelog entry to the eslint-plugin package?

packages/eslint-plugin/configs/recommended.js

test/performance/config/performance-reporter.ts

packages/components/src/palette-edit/test/index.tsx

packages/eslint-plugin/configs/recommended.js

Co-authored-by: Jon Surrell <sirreal@users.noreply.github.com>

sirreal · 2024-07-01T17:28:14Z

Let's also remove the noUnused* config from all the package tsconfigs where it's redundant now.

packages/eslint-plugin/CHANGELOG.md

Co-authored-by: Jon Surrell <sirreal@users.noreply.github.com>

sirreal

Thanks for the thoughtful discussion. I'm ready to approve and land this pending two changes:

Revert and eslint-disable this valid case: Scripts: Ensure that typescript-eslint checks for unused vars #62925 (comment)
Remove other tsconfig noUnused*: Scripts: Ensure that typescript-eslint checks for unused vars #62925 (comment)

Chrico · 2024-07-05T06:39:16Z

Revert and eslint-disable this valid case: Scripts: Ensure that typescript-eslint checks for unused vars #62925 (comment)

Change was done here: 2b21b06

Remove other tsconfig noUnused*: Scripts: Ensure that typescript-eslint checks for unused vars #62925 (comment)

Updates were done here: 93da83f

editor/dataviews // remove unused vars

sirreal

Thanks, nice work on this one @Chrico.

gziolo · 2024-07-08T06:53:14Z

Thank you both for taking it to the finish line 🥇

…#62925) Enable @typescript-eslint/no-unused-vars eslint rule in default eslint TypeScript configuration. Disable redundant `noUnusedLocals` and `noUnusedParameters` from TypeScript configuration. The TypeScript eslint configuration disabled no-unused-vars rules, resulting in many cases of unused vars appearing in code. The expectation was that TypeScript handle the linting via configuration such as `noUnusedLocals` and `noUnusedParameters`. This was unreliable because TypeScript may not run on entire projects, for example if `checkJs` is disabled. --- Co-authored-by: Chrico <chrico@git.wordpress.org> Co-authored-by: sirreal <jonsurrell@git.wordpress.org> Co-authored-by: gziolo <gziolo@git.wordpress.org>

Chrico requested review from gziolo, ntwb, nerrad and ajitbohra as code owners June 27, 2024 13:03

github-actions bot added the First-time Contributor Pull request opened by a first-time contributor to Gutenberg repository label Jun 27, 2024

Chrico changed the title ~~wp-scripts lint-js - ensure that typescript type checker is executed.~~ Jun 27, 2024

gziolo added [Package] ESLint plugin /packages/eslint-plugin [Type] Enhancement A suggestion for improvement. labels Jun 28, 2024

recommended.js // disalbe "no-unused-vars" and just enable "@typescri…

a4a9cad

…pt-eslint/no-unused-vars"

Chrico added 3 commits June 28, 2024 12:27

cleanup @typescript-eslint/unused-vars errors

d152715

cleanup @typescript-eslint/unused-vars errors.

3e4b619

performance-reporter.ts // formatting

4057bc5

token-list // disable and enable eslint "no-unused-var" for supports()-method.

cleanup @typescript-eslint/unused-vars errors.

c25e163

Chrico requested a review from kevin940726 as a code owner June 28, 2024 15:16

gziolo changed the title ~~wp-scripts lint-js - ensure that typescript eslint is executed.~~ Jun 29, 2024

sirreal requested changes Jul 1, 2024

View reviewed changes

Update test/performance/config/performance-reporter.ts

bdb8503

Co-authored-by: Jon Surrell <sirreal@users.noreply.github.com>

Chrico added 3 commits July 1, 2024 12:16

recommended.js // update no-unused-vars explanation.

94ed5a3

tsconfig.base.json // removed "noUnusedLocals" and "noUnusedParameters"

92944da

eslint-plugin // update changelog

e3ac5aa

sirreal reviewed Jul 1, 2024

View reviewed changes

packages/eslint-plugin/CHANGELOG.md Outdated Show resolved Hide resolved

Update packages/eslint-plugin/CHANGELOG.md

ff14a28

Co-authored-by: Jon Surrell <sirreal@users.noreply.github.com>

sirreal reviewed Jul 4, 2024

View reviewed changes

Chrico added 2 commits July 5, 2024 06:37

palette-edit // revert changes and add eslint-disable.

2b21b06

tsconfig // remove "noUnusedParameters"

93da83f

Chrico added 2 commits July 5, 2024 08:42

Merge branch 'trunk' into 54305

fd3bf17

dataviews/dataform // remove unused vars

033a4b7

editor/dataviews // remove unused vars

sirreal approved these changes Jul 5, 2024

View reviewed changes

sirreal merged commit 938a513 into WordPress:trunk Jul 5, 2024
60 of 61 checks passed

github-actions bot added this to the Gutenberg 18.8 milestone Jul 5, 2024

Chrico mentioned this pull request Jul 5, 2024

@wordpress/scripts: Improve TypeScript support in lint-js #54305

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Scripts: Ensure that typescript-eslint checks for unused vars #62925

Scripts: Ensure that typescript-eslint checks for unused vars #62925

Chrico commented Jun 27, 2024

github-actions bot commented Jun 27, 2024 •

edited

Loading

github-actions bot commented Jun 27, 2024

gziolo commented Jun 28, 2024

Chrico commented Jun 28, 2024 •

edited

Loading

gziolo commented Jun 28, 2024

Chrico commented Jun 28, 2024

gziolo commented Jun 28, 2024

Chrico commented Jun 28, 2024 •

edited

Loading

gziolo commented Jun 28, 2024

Chrico commented Jun 28, 2024

gziolo commented Jun 28, 2024

Chrico commented Jun 28, 2024

sirreal left a comment

sirreal commented Jul 1, 2024 •

edited

Loading

sirreal left a comment

Chrico commented Jul 5, 2024

sirreal left a comment

gziolo commented Jul 8, 2024

Scripts: Ensure that typescript-eslint checks for unused vars #62925

Scripts: Ensure that typescript-eslint checks for unused vars #62925

Conversation

Chrico commented Jun 27, 2024

What?

Why?

Testing Instructions

github-actions bot commented Jun 27, 2024 • edited Loading

github-actions bot commented Jun 27, 2024

gziolo commented Jun 28, 2024

Chrico commented Jun 28, 2024 • edited Loading

gziolo commented Jun 28, 2024

Chrico commented Jun 28, 2024

gziolo commented Jun 28, 2024

Chrico commented Jun 28, 2024 • edited Loading

gziolo commented Jun 28, 2024

Chrico commented Jun 28, 2024

gziolo commented Jun 28, 2024

Chrico commented Jun 28, 2024

sirreal left a comment

Choose a reason for hiding this comment

sirreal commented Jul 1, 2024 • edited Loading

sirreal left a comment

Choose a reason for hiding this comment

Chrico commented Jul 5, 2024

sirreal left a comment

Choose a reason for hiding this comment

gziolo commented Jul 8, 2024

github-actions bot commented Jun 27, 2024 •

edited

Loading

Chrico commented Jun 28, 2024 •

edited

Loading

Chrico commented Jun 28, 2024 •

edited

Loading

sirreal commented Jul 1, 2024 •

edited

Loading