[Feature Request]: Check TypeScript types

[shvlv]

Is your feature request related to a problem?

Not really. But now if have a TypeScript-based project there is no possibility to verify the type system before asset compiling. This means we can't leverage TypeScript checks during PRs review.

Describe the desired solution

The simplest solution is to run such a command: tsc --noEmit -project ./tsconfig.eslint.json --skipLibCheck. Sure will be nice to inline errors in the files view. But my quick GithHub search there is no maintained and ready-to-use GitHub action.

Maybe we could check https://github.com/Arhia/action-check-typescript but there are a lot of issues without a respond.

So IMO small dependency-less command is better.

Describe the alternatives that you have considered

Do nothing.

Additional context

No response

Code of Conduct

• [X] I agree to follow this project's Code of Conduct

[widoz]

I've seen there are a couple of interesting arguments such as

• noErrorTruncation
• noUnusedLocals
• noUnusedParameters
• strictFunctionTypes
• ...

There might be some probles in some .tsx files by importing React in a UMD context because you could obtain the following error 👇. Nonetheless, the noUnusedLocals is a good thing because allow you to spot dead code.

Note: Removing the import might not be the solution because the import has to do with the UMD React error. More info at https://www.totaltypescript.com/react-refers-to-a-umd-global

modules/components/resources/js/block-formats/rich-text/index.tsx:1:1 - error TS6133: 'React' is declared but its value is never read.

1 import React from 'react'
  ~~~~~~~~~~~~~~~~~~~~~~~~~

In this case I think it's possible to resolve the problem by changing the tsconfig.json and remove the react import.

{
    "compilerOptions": {
        "jsx": "react-jsx",
   }
}

---

Have you also considered to have tsc as internal dev dep instead of installed globally?

I agree if a dep can be avoided the better.

[Chrico]

We could install the tsc dependency globally in the reusable workflow itself via npm install -g typescript - but normally you should have typescript defined in your package.json, right?

I'm wondering if we can solve the actual problem here. While we can implement a reusable-workflow lint-typescript.yml, what about SCSS and other things which are also required for "successfully building assets"?

I've following scenario:

Files

test.scss

foobarbaz []

test.ts

import '../scss/test.scss';
class Foo {
	constructor( private bar: string ) {}
}

new Foo( 1 );

---

"Linting"

1. Running TSC

$ tsc --noEmit --skipLibCheck resources/ts/*
resources/ts/test.ts:6:10 - error TS2345: Argument of type 'number' is not assignable to parameter of type 'string'.

6 new Foo( 1 );
           ~
Found 1 error in resources/ts/test.ts:6

2. Running yarn build

$ yarn build

// SNIP

Error: Module build failed (from ./node_modules/sass-loader/dist/cjs.js):
SassError: expected "{".
  ╷
1 │ foobarbaz []
  │             ^
  ╵
  resources\scss\test.scss 1:13  root stylesheet

// SNIP

./resources/ts/test.ts 6:9-10
[tsl] ERROR in {snip}\resources\ts\test.ts(6,10)
      TS2345: Argument of type 'number' is not assignable to parameter of type 'string'.

2 errors have detailed information that is not shown.
Use 'stats.errorDetails: true' resp. '--stats-error-details' to show it.

webpack 5.88.2 compiled with 3 errors in 1684 ms

CC @tyrann0us

[shvlv]

We could install the tsc dependency globally in the reusable workflow itself via npm install -g typescript - but normally you should have typescript defined in your package.json, right?

Yes, it doesn't make sense to check types in the project without Typescript. The issue is not about installing Typescript but about the command to check types.

I'm wondering if we can solve the actualy problem here. While we can implement a reusable-workflow lint-typescript.yml, what about SCSS and other things which are also required for "successfully building assets"?

Let's think about Typescript in the same ways as Psalm for PHP.

[tyrann0us]

We raised this upstream in https://github.com/WordPress/gutenberg/issues/54305, which ideally will result in a new command for @wordpress/scripts, e.g., lint-ts. Let's await feedback there (CC @karmatosed); putting this on hold for the time being.

[Chrico]

Just to give a short update. It seems like Gutenberg @wordpress/eslint-plugin has support out of the box for Typescript and already installs the right plugins: gutenberg/packages/eslint-plugin#usage. But because of a configuration error no type checks are excuted. To get around for it right now you can do following in your package.json:

    "eslintConfig": {
        "rules": {
            "no-unused-vars": "off",
            "@typescript-eslint/no-unused-vars": "error",
        }
    }

I'm currently in discussion in https://github.com/WordPress/gutenberg/issues/54305 , why they do "no-unused-vars": "off" in the default configuration.

[shvlv]

I've rechecked the issue and tested https://github.com/WordPress/gutenberg/pull/62925 alongside pure https://typescript-eslint.io/ locally and in the GitHub action environment. It doesn't fix the original problem of checking types during linting. The https://github.com/typescript-eslint/typescript-eslint/issues/352#issuecomment-472597115 issue explains why. typescript-eslint does not check types by design; the plugin purpose is to enforce codebase consistent based on some opinionated rules like const x: Array<string> = ['a', 'b']; is incorrect but const x: string[] = ['a', 'b']; (we can consider using some of those rules, but there are not strong benefits).

Moreover, the mentioned issue suggests to use TS compiler:

If you want your code to be typechecked - please use tsc (or one of the tools built for your specific build chain).

The same can be found in the documentation - https://typescript-eslint.io/rules/typedef/:

Instead of enabling typedef, it is generally recommended to use the --noImplicitAny and --strictPropertyInitialization compiler options to enforce type annotations only when useful.

So in the end, using of tsc --noEmit is the single way to do typecheck before the code gets merged to the main branch.

Regarding additional compiler options I think the best if we would run the type check command with the same options as we have in tsconfig.json. But if we are talking about enforcing of stricter types usage we can use noImplicitAny and strictPropertyInitialization. But this is might be project-specific and it's outside of the current issue.

[tyrann0us]

Thank you for your investigation! As discussed, please investigate the feasibility/possibility of adding typechecking to Gutenberg or rather @wordpress/scripts. Thank you!

[shvlv]

I've invested time in checking how type-checking happens inside Gutenberg.

The Gutenberg repository has "Static Analysis (Linting, License, Type checks...)" workflow with "Type checking" part - https://github.com/WordPress/gutenberg/blob/2e967be9e1928809841761744b4ed8c768f0a417/.github/workflows/static-checks.yml#L48-L49. This step build:package-types run tsc --build command. The command fails if type definitions are incorrect.

Therefore, if typescript-eslint maintainers suggest using the Typescript compiler and the Gutenberg project uses it, I don't see the reason not to use it to check types.

Note to say the way the Gutenberg project and Syde company use Typescript is very different. We use ts-loader instead of babel-loader, which comes with @wordpress/scripts. So Gutenberg uses Typescript compiler only to build types (note declaration and emitDeclarationOnly in the base config file - https://github.com/WordPress/gutenberg/blob/938a51387ff6c1b7f8c7a4ad4deb745afeff84d5/tsconfig.base.json#L11-L15 and allowJs - https://github.com/WordPress/gutenberg/blob/938a51387ff6c1b7f8c7a4ad4deb745afeff84d5/tsconfig.base.json#L4). So Gutenberg generates types for JS files as well and consumers can use Typescript definitions extracting from JS doc.