Skip to content
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 our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 15 commits into from
Jul 5, 2024

Conversation

Chrico
Copy link
Contributor

@Chrico Chrico commented Jun 27, 2024

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

github-actions bot commented Jun 27, 2024

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.

Copy link

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

@github-actions github-actions bot added the First-time Contributor Pull request opened by a first-time contributor to Gutenberg repository label Jun 27, 2024
@Chrico Chrico changed the title wp-scripts lint-js - ensure that typescript type checker is executed. wp-scripts lint-js - ensure that typescript eslint is executed. Jun 27, 2024
@gziolo gziolo added [Tool] ESLint plugin /packages/eslint-plugin [Type] Enhancement A suggestion for improvement. labels Jun 28, 2024
@gziolo
Copy link
Member

gziolo commented Jun 28, 2024

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

Screenshot 2024-06-28 at 08 27 25

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

Chrico commented Jun 28, 2024

@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 ;-)

@gziolo
Copy link
Member

gziolo commented Jun 28, 2024

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

Screenshot 2024-06-28 at 11 06 46

@Chrico
Copy link
Contributor Author

Chrico commented Jun 28, 2024

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

gziolo commented Jun 28, 2024

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

Chrico commented Jun 28, 2024

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

gziolo commented Jun 28, 2024

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

@Chrico
Copy link
Contributor Author

Chrico commented Jun 28, 2024

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

gziolo commented Jun 28, 2024

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

@Chrico Chrico requested a review from kevin940726 as a code owner June 28, 2024 15:16
@Chrico
Copy link
Contributor Author

Chrico commented Jun 28, 2024

Here we go @gziolo :) ☕

@gziolo gziolo changed the title wp-scripts lint-js - ensure that typescript eslint is executed. Scripts: Ensure that typescript-eslint checks for unused vars Jun 29, 2024
Copy link
Member

@sirreal sirreal left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 Outdated Show resolved Hide resolved
test/performance/config/performance-reporter.ts Outdated Show resolved Hide resolved
packages/components/src/palette-edit/test/index.tsx Outdated Show resolved Hide resolved
packages/eslint-plugin/configs/recommended.js Show resolved Hide resolved
Co-authored-by: Jon Surrell <sirreal@users.noreply.github.com>
@sirreal
Copy link
Member

sirreal commented Jul 1, 2024

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

@sirreal sirreal left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@Chrico
Copy link
Contributor Author

Chrico commented Jul 5, 2024

Change was done here: 2b21b06

Updates were done here: 93da83f

Copy link
Member

@sirreal sirreal left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, nice work on this one @Chrico.

@sirreal sirreal merged commit 938a513 into WordPress:trunk Jul 5, 2024
60 of 61 checks passed
@github-actions github-actions bot added this to the Gutenberg 18.8 milestone Jul 5, 2024
@gziolo
Copy link
Member

gziolo commented Jul 8, 2024

Thank you both for taking it to the finish line 🥇

carstingaxion pushed a commit to carstingaxion/gutenberg that referenced this pull request Jul 18, 2024
…#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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
First-time Contributor Pull request opened by a first-time contributor to Gutenberg repository [Tool] ESLint plugin /packages/eslint-plugin [Type] Enhancement A suggestion for improvement.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants