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

Chrico · 2023-09-08T13:45:00Z

What problem does this address?

Right now the @wordpress/scripts package provides lint-js (eslint), lint-style (stylelint), lint-md-docs (markdownlint) and lint-pkg-json (npmPkgJsonLint).

While tools like lint-style via stylelint are able to find syntax errors - as an example:

index.scss

foo []

$ yarn wp-scripts lint-style
yarn run v1.22.19

resources/scss/index.scss
 1:1  ✖  Unknown word  CssSyntaxError

1 problem (1 error, 0 warnings)

the lint-js does not actually check typescript code to detect type errors. As an example:

index.ts

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

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

$ yarn wp-scripts lint-js
yarn run v1.22.19
Done in 2.10s.

Running tsc with --noEmit will detect an error:

 tsc --noEmit --skipLibCheck resources/ts/*

resources/ts/index.ts:14:10 - error TS2345: Argument of type 'number' is not assignable to parameter of type 'string'.

14 new Foo( 1 );
            ~

Found 1 error in resources/ts/index.ts:14

What is your proposed solution?

Introduce a new lint-ts script in @wordpress/scripts which does lint Typescript code. To do that we can make use of the tsc with --noEmit to do only the type checking part and not the compilation.

See also: https://www.typescriptlang.org/tsconfig#noEmit

The text was updated successfully, but these errors were encountered:

Chrico · 2023-09-11T09:10:15Z

Just wanted to add some "findings". It seems like the @wordpress/eslint-plugin mentions support for Typescript

Finally, this ruleset also includes an optional integration with the @typescript-eslint/eslint-plugin package that enables ESLint to support TypeScript language. You can activate it by installing the typescript package separately with:

See here: gutenberg/packages/eslint-plugin#usage

It looks like there is already some support for Typescript according due: gutenberg/packages/eslint-plugin/configs/recommended.js#L31-L60

Edit:

It looks like this line here:

gutenberg/packages/eslint-plugin/configs/recommended.js

Lines 52 to 53 in 65bf4f7

    
           // Handled by TS itself. 
        
           'no-unused-vars': 'off',

disables the Typescript type check. Any idea why this is the case/was decided? 🤔

Link: https://typescript-eslint.io/rules/no-unused-vars/

gziolo · 2023-09-11T11:10:20Z

wp-scripts lint-js should work with TypeScript, too. You only need to ensure that typescript is installed in your project. We can add an alias like wp-scripts lint-ts, but the only difference would be that it verifies that typescript is installed in the project.

Chrico · 2023-09-11T12:21:01Z

wp-scripts lint-js should work with TypeScript, too. You only need to ensure that typescript is installed in your project. We can add an alias like wp-scripts lint-ts, but the only difference would be that it verifies that typescript is installed in the project.

Thanks for your reply 👍🏻 Typescript is installed and actually the isPackageInstalled( 'typescript' ) check in gutenberg/packages/eslint-plugin/configs/recommended.js is returning "true" and the configuration is added. The problem is that L53 is not allowing to execute any Typescript type checks.

https://github.com/typescript-eslint/typescript-eslint/blob/main/packages/eslint-plugin/docs/rules/no-unused-vars.md

This rule extends the base eslint/no-unused-vars rule. It adds support for TypeScript features, such as types.

So by turning this off...no Typescript type checks are done.

As it is:

$ wp-scripts lint-js resources/ts/**/*.{ts,tsx}
yarn run v1.22.19

Done in 1.83s.

With L53 commented out:

$ wp-scripts lint-js resources/ts/**/*.{ts,tsx}
yarn run v1.22.19

/{snip}/tsc-eslint\resources\ts\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.

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"
    }
}

gziolo · 2023-09-11T14:25:06Z

Thanks for your reply 👍🏻 Typescript is installed and actually the isPackageInstalled( 'typescript' ) check in gutenberg/packages/eslint-plugin/configs/recommended.js is returning "true" and the configuration is added. The problem is that L53 is not allowing to execute any Typescript type checks.

https://github.com/typescript-eslint/typescript-eslint/blob/main/packages/eslint-plugin/docs/rules/no-unused-vars.md

This rule extends the base eslint/no-unused-vars rule. It adds support for TypeScript features, such as types.

So by turning this off...no Typescript type checks are done.

As it is:
$ wp-scripts lint-js resources/ts/**/*.{ts,tsx}
yarn run v1.22.19

Done in 1.83s.
With L53 commented out:
$ wp-scripts lint-js resources/ts/**/*.{ts,tsx}
yarn run v1.22.19

/{snip}/tsc-eslint\resources\ts\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.
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"
    }
}

eslint/no-unused-vars got disabled when introducing support for TypeScript with #27143 (2-3 years ago). It's very likely that the way it's handled has changed, and we should have the rule enabled again, as there isn't a rule coming from typescript-eslint anymore, but instead the plugin overrides the default ESLint rule.

Chrico · 2023-09-12T05:47:15Z

eslint/no-unused-vars got disabled when introducing support for TypeScript with #27143 (2-3 years ago). It's very likely that the way it's handled has changed, and we should have the rule enabled again, as there isn't a rule coming from typescript-eslint anymore, but instead the plugin overrides the default ESLint rule.

Yeah, already thought about such a thing like "Gutenberg was not fully using Typescript and disabled it". Thanks for confirmation.

Since you're also using tsc for various scenarios to build your code https://github.com/WordPress/gutenberg/blob/trunk/package.json#L267 and the tsconfig.json already defines strict type checks, I guess it makes sense to remove that configuration line in @wordpress/eslint-plugin and allow wp-scripts lint-js to use fully eslint-typescript-plugin and do type checks?!

gziolo · 2023-09-12T06:43:04Z

Gutenberg uses typescript-eslint for linting. The one slight difference I noticed is that it uses eslint-recommended configuration. I believe that is different from the recommeded configuration they have, and I can see they handle no-unesed-vars this way:

https://github.com/typescript-eslint/typescript-eslint/blob/548fe2d84125c72580939fbe763870d597c9af4c/packages/eslint-plugin/src/configs/recommended.ts#L26-L27

In fact, it also extends eslint-recommended:

https://github.com/typescript-eslint/typescript-eslint/blob/548fe2d84125c72580939fbe763870d597c9af4c/packages/eslint-plugin/src/configs/recommended.ts#L9

It'd probably be best to switch to the recommended configuration from typescript-eslint. The question is whether it's feasible in the Gutenberg project because some parts of the codebase don't use TypeScript.

gziolo · 2023-09-12T07:33:10Z

I tried the following:

diff --git a/packages/eslint-plugin/configs/recommended.js b/packages/eslint-plugin/configs/recommended.js
index 8f0399e8e1..7ef78a2b6a 100644
--- a/packages/eslint-plugin/configs/recommended.js
+++ b/packages/eslint-plugin/configs/recommended.js
@@ -49,11 +49,11 @@ if ( isPackageInstalled( 'typescript' ) ) {
 				// Don't require redundant JSDoc types in TypeScript files.
 				'jsdoc/require-param-type': 'off',
 				'jsdoc/require-returns-type': 'off',
-				// Handled by TS itself.
-				'no-unused-vars': 'off',
-				// no-shadow doesn't work correctly in TS, so let's use a TS-dedicated version instead.
+				// Following rules don't work correctly in TS, so let's use a TS-dedicated version instead.
 				'no-shadow': 'off',
+				'no-unused-vars': 'off',
 				'@typescript-eslint/no-shadow': 'error',
+				'@typescript-eslint/no-unused-vars': 'error',
 			},
 		},
 	];

It reports 42 errors, but they seem to be legit.

Chrico · 2023-09-12T08:44:04Z

It reports 42 errors, but they seem to be legit.

I guess this is a blocker to "activate" it straight away, because it would break every current PR to run successful?! 😅 Can you quickly post the errors here so that everyone can see if those are "easy to fix" or maybe need some more time invested?

gziolo · 2023-09-12T09:01:18Z

I don't think it's a blocker. We can always signal a breaking change for @wordpress/eslint-plugin when enabling this rule. The only challenge I see is that we often use unused vars for the equivalent of omit from Lodash:

const { ignoredVar, ...restArgs } = args;

doSomething( restArgs );

@tyxla, do you know if there is a better way to signal to ESLint that ignoredVar is there for a reason other than using the code comment to disable the error from @typescript-eslint/no-unused-vars?

I also saw some reports in TS definitions when using generics which I'm not sure if should be reported.

tyxla · 2023-09-12T10:56:17Z

@tyxla, do you know if there is a better way to signal to ESLint that ignoredVar is there for a reason other than using the code comment to disable the error from @typescript-eslint/no-unused-vars?

AFAIK @typescript-eslint/no-unused-vars extends the no-unused-vars rule, so you should be able to provide ignoreRestSiblings: true to let the linter skip those unused destructured variables. Haven't tested, but you should be able to do something like this as part of your rules configuration option:

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

Let me know if that works for you.

Chrico · 2023-09-19T05:56:31Z

Any updates here regarding moving that topic forward? @tyxla @gziolo

tyxla · 2023-09-19T10:33:37Z

My understanding is that the direction here is clear and this just needs a developer to pick it up and open a PR, but I might be missing something, so I'll defer to @gziolo.

gziolo · 2023-09-19T10:46:20Z

Yes, this issue is waiting for any developer to enable the rule as outlined by @tyxla in #54305 (comment) in packages/eslint-plugin/configs/recommended.js:

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

…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

Chrico · 2024-07-05T09:56:23Z

This issue was resolved with #62925

CC @sirreal

Chrico mentioned this issue Sep 8, 2023

[Feature Request]: Check TypeScript types inpsyde/reusable-workflows#63

Closed

1 task

mikachan added the Developer Experience Ideas about improving block and theme developer experience label Sep 8, 2023

gziolo added [Tool] WP Scripts /packages/scripts [Type] New API New API to be used by plugin developers or package users. labels Sep 8, 2023

gziolo added the [Tool] ESLint plugin /packages/eslint-plugin label Sep 12, 2023

gziolo added the Needs Dev Ready for, and needs developer efforts label Sep 14, 2023

gziolo added [Type] Enhancement A suggestion for improvement. and removed [Type] New API New API to be used by plugin developers or package users. labels Sep 20, 2023

gziolo changed the title ~~@wordpress/scripts - add lint-ts~~ @wordpress/scripts: Improve TypeScript support in lint-js Sep 20, 2023

colorful-tones mentioned this issue Apr 25, 2024

@wordpress/scripts lint-js not compatible with newer versions of ESLint (ESLint 9+) #55499

Closed

Chrico mentioned this issue Jun 27, 2024

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

Merged

Chrico closed this as completed Jul 5, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

Chrico commented Sep 8, 2023

Chrico commented Sep 11, 2023 •

edited

Loading

gziolo commented Sep 11, 2023

Chrico commented Sep 11, 2023 •

edited

Loading

gziolo commented Sep 11, 2023 •

edited

Loading

Chrico commented Sep 12, 2023

gziolo commented Sep 12, 2023 •

edited

Loading

gziolo commented Sep 12, 2023

Chrico commented Sep 12, 2023

gziolo commented Sep 12, 2023

tyxla commented Sep 12, 2023

Chrico commented Sep 19, 2023

tyxla commented Sep 19, 2023

gziolo commented Sep 19, 2023

Chrico commented Jul 5, 2024

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

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

Comments

Chrico commented Sep 8, 2023

What problem does this address?

What is your proposed solution?

Chrico commented Sep 11, 2023 • edited Loading

gziolo commented Sep 11, 2023

Chrico commented Sep 11, 2023 • edited Loading

gziolo commented Sep 11, 2023 • edited Loading

Chrico commented Sep 12, 2023

gziolo commented Sep 12, 2023 • edited Loading

gziolo commented Sep 12, 2023

Chrico commented Sep 12, 2023

gziolo commented Sep 12, 2023

tyxla commented Sep 12, 2023

Chrico commented Sep 19, 2023

tyxla commented Sep 19, 2023

gziolo commented Sep 19, 2023

Chrico commented Jul 5, 2024

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

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

Chrico commented Sep 11, 2023 •

edited

Loading

Chrico commented Sep 11, 2023 •

edited

Loading

gziolo commented Sep 11, 2023 •

edited

Loading

gziolo commented Sep 12, 2023 •

edited

Loading