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

[Feature Request]: Check TypeScript types #63

Open
1 task done
shvlv opened this issue Aug 16, 2023 · 5 comments
Open
1 task done

[Feature Request]: Check TypeScript types #63

shvlv opened this issue Aug 16, 2023 · 5 comments
Labels
enhancement New feature or request on hold Pull requests that update a dependency file

Comments

@shvlv
Copy link
Contributor

shvlv commented Aug 16, 2023

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

  • I agree to follow this project's Code of Conduct
@shvlv shvlv added the enhancement New feature or request label Aug 16, 2023
@widoz
Copy link
Member

widoz commented Aug 16, 2023

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

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

Chrico commented Aug 30, 2023

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

shvlv commented Aug 30, 2023

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

We raised this upstream in WordPress/gutenberg#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.

@tyrann0us tyrann0us added the on hold Pull requests that update a dependency file label Sep 8, 2023
@Chrico
Copy link
Member

Chrico commented Sep 11, 2023

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 WordPress/gutenberg#54305 , why they do "no-unused-vars": "off" in the default configuration.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request on hold Pull requests that update a dependency file
Projects
None yet
Development

No branches or pull requests

4 participants