Scripts: Add TypeScript support to linting command #27143
Conversation
|
Size Change: +6.66 kB (0%) Total Size: 1.39 MB
ℹ️ View Unchanged
|
gziolo
added
the
[Type] Automated Testing
sirreal
force-pushed
the
update/lint-staged-ts
branch
from
f18c6f5 to
f1fe9fe
Compare
packages/eslint-plugin/package.json
Outdated
| @@ -31,12 +31,14 @@ | |||
| ], | |||
| "main": "index.js", | |||
| "dependencies": { | |||
| "@typescript-eslint/eslint-plugin": "^4.8.2", | |||
| "@typescript-eslint/parser": "^4.8.2", | |||
There was a problem hiding this comment.
Parser is required for TS syntax.
|
This is where we are when calling |
sirreal
force-pushed
the
update/lint-staged-ts
branch
from
02108a5 to
629ece7
Compare
|
I fixed up some lint issues just to demo this. The when committing the react-i18n changes, lint-stages reports: |
|
This is failing because some existing TS files have lint errors. There seem to be a lot of formatting issues at least. I'd appreciate if anyone wants to help with the cleanup. |
sirreal
added
[Tool] ESLint plugin
How many errors are we talking about? Should we start with enabling linting for TS only in one of the packages to keep this PR small? Do you feel comfortable allowing TS linting in |
| */ | ||
| // Disable reason: Type-only import, this is fine. See https://github.com/typescript-eslint/typescript-eslint/issues/2661 | ||
| // eslint-disable-next-line no-restricted-imports | ||
| import type { ComponentType, PropsWithChildren } from 'react'; |
There was a problem hiding this comment.
It depends how you look at this ESLint warning 😃 We bring React API from @wordpress/element that acts as a proxy. It wouldn't be that bad idea to re-export React types used from @wordpress/element to align with how you use code.
There was a problem hiding this comment.
I agree up to a point.
We looked at doing that and it was a nightmare at the time and would've required a lot of maintenance effort for little or no benefit. There's no way to work nicely with types in the module import/export system in JSDoc. We would've had to manually re-export every single type (with their type arguments!) one by one via JSDoc.
Now that we're allowing some TypeScript, it may be more feasible to re-export all the types, but I haven't looked in many months. It still won't be as easy as something like export type * from 'react' as far as I know.
There was a problem hiding this comment.
Unless there is some way to let ESLint know that it should give itself a break when seeing import type when processing no-restricted-imports 🤔
Let's see: eslint/eslint#13758, typescript-eslint/typescript-eslint#2661.
There was a problem hiding this comment.
Maybe we should list React as peer dependency in that case? There are more packages that would require the same handling.
There was a problem hiding this comment.
I reviewed all the changes again.
It won't scale with so many types required from React or Reakit 🙁
Should we at least disable this rule for all packages/**/types.ts files? Ideally, the rule shouldn't error for type imports in the first place.
There was a problem hiding this comment.
I think we should just disable those rules for packages/components... G2 imports a lot from reakit as well, outside of typescript files.
There was a problem hiding this comment.
We could as well remove those checks for react and reakit if that becomes an issue. Although, my initial comment still applies. Technically speaking both those libraries are implementation details and shouldn't leak outside of @wordpress/element (react) and @wordpress/components (reakit). If there is no way to re-export the types that are used in other packages, then we shouldn't fight with the linter and disable the rule as a primary solution. We better remove the rule that becomes hostile.
sirreal
force-pushed
the
update/lint-staged-ts
branch
from
4b55261 to
b463f13
Compare
|
@sirreal There are a lot of unrelated linting errors in this PR right? |
There are 47 errors reported and as far as I can tell they are valid issues with TS files. |
Oh gotcha, I was mixing up errors and warnings! @sirreal I'm happy to tackle fixing the errors in the TS files. |
|
Hmmm, it looks like there's something funky going on here that I'm not quite sure of. Following the trail of errors from Also when I ran Something further is even more wrong. For the following code in export type Options< T extends As, P extends ViewOwnProps< {}, T > > = {
as: T;
name: string;
useHook: ( props: P ) => any;
memo?: boolean;
};I am seeing the following error:
However, |
I'm not sure it is a breaking change, but I guess if someone had been using |
|
Yes, it might be an excellent reason to be cautious in case someone has integrated TS support themselves. That makes sense 👍🏻 In fact, we won't be able to publish a new version of both packages to npm until WordPress 5.7 is out which is on Match 9th, so we still might add more breaking changes to the mix 😄 |
|
My reasoning for the breaking change is that:
Either could mean that the upgrade requires some manual work to get this ready and is worth mentioning so folks no what to expect. Anything that may require work on the consumer's end to upgrade is my understanding of a breaking change.
I certainly don't know whether this is perfect, but it's at a point where it can start getting exercise. If we're blocked on package releases can we |
|
I can't approve this because I opened it, although @sarayourfriend has done a lot of the work 🙇♂️ . I think it's ready to be merged and see how it performs. |
Sure thing, we can do it as soon as there is the next Gutenberg plugin RC ready 👍🏻 |
We're implementing TS in WooCommerce Blocks feature plugin and definitely could benefit from this update (which looks good afaict). |
gziolo
changed the title
…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
Description
With some TypeScript in Gutenberg, .ts and .tsx (TypeScript) files should undergo the similar formatting and linting to
.jsfiles.How has this been tested?
Change a .ts file, stage, and commit it.
Work on
@wordpress/react-i18nand see that lint issues are reported correctly.Types of changes
Breaking change: Add lint setup and rules for TypeScript files.
Checklist: