Size Change: +6.66 kB (0%)

Total Size: 1.39 MB

_{compressed-size-action}

This is where we are when calling npm run lint-js -- --quiet:

The prop value with an expression type of JSXFragment could not be resolved. Please file issue to get this fixed immediately.

/Users/gziolo/Projects/gutenberg/packages/data/src/types.ts
   7:15  error  'listener' is defined but never used  no-unused-vars
  19:17  error  'registry' is defined but never used  no-unused-vars
  23:13  error  'state' is defined but never used     no-unused-vars
  23:25  error  'action' is defined but never used    no-unused-vars
  31:14  error  'store' is defined but never used     no-unused-vars

/Users/gziolo/Projects/gutenberg/packages/editor/src/components/post-saved-state/index.js
  79:26  error  ["_links"] is better written in dot notation  dot-notation

/Users/gziolo/Projects/gutenberg/packages/editor/src/components/post-trash/check.js
  21:31  error  ["rest_base"] is better written in dot notation  dot-notation

✖ 7 problems (7 errors, 0 warnings)
  2 errors and 0 warnings potentially fixable with the `--fix` option.

I fixed up some lint issues just to demo this. The when committing the react-i18n changes, lint-stages reports:

husky > pre-commit (node v14.15.5)
  ↓ Stashing changes... [skipped]
    → No partially staged files found...
  ❯ Running tasks...
    ↓ Running tasks for package-lock.json [skipped]
      → No staged files match package-lock.json
    ↓ Running tasks for packages/*/package.json [skipped]
      → No staged files match packages/*/package.json
    ↓ Running tasks for *.scss [skipped]
      → No staged files match *.scss
    ❯ Running tasks for *.{js,ts,tsx}
      ✔ wp-scripts format-js
      ✖ wp-scripts lint-js
    ↓ Running tasks for {docs/{toc.json,tool/*.js},packages/{*/README.md,components/src/*/**/README.md}} [skipped]
      → No staged files match {docs/{toc.json,tool/*.js},packages/{*/README.md,components/src/*/**/README.md}}
    ✔ Running tasks for packages/**/*.{js,ts,tsx}



✖ wp-scripts lint-js found some errors. Please fix them and try committing again.

…/gutenberg/packages/react-i18n/src/index.tsx
   30:1  error  Missing JSDoc @param "i18n" declaration     jsdoc/require-param
   48:1  error  Missing JSDoc @param "props" declaration    jsdoc/require-param
  116:0  error  Missing JSDoc @param "InnerComponent" type  jsdoc/require-param-type
  117:0  error  Missing JSDoc @return type                  jsdoc/require-returns-type

✖ 4 problems (4 errors, 0 warnings)
  2 errors and 0 warnings potentially fixable with the `--fix` option.

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.

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.

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 @wordpress/scripts? It's perfectly fine to add linting for files with TS extensions, but the rest might wait. I'm just checking since it means breaking changes into two packages 😃

@sirreal There are a lot of unrelated linting errors in this PR right?

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

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 control-label/types we get an error about not being able to resolve ../text/types. If we go to that module, which exists at that correct path, we see that it has an error about not being able to resolve ../truncate/types. That module also exists at that path and itself has no errors. I'm not sure why eslint is unable to resolve these moduels 🤔

Also when I ran npm install on this branch I ended up with loads of package-lock.json changes.

Something further is even more wrong. For the following code in components/ui/utils/types:

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:

'props' is defined but never used. eslint(no-unused-vars)

However, props there is just a type annotation. Is something wrong with @typescript-eslint/parser?

It's erroring in packages/dependency-extraction-webpack-plugin/lib/types.d.ts? Eslint always reports errors for me in .d.ts files. It looks like it should use the correct parser and not fail to parse, but 🤷 .

This is the failing line:

export = DependencyExtractionWebpackPlugin;

I don't think we should eslint lint there, it's not really an appropriate tool for linting declaration files. I think we should add *.d.ts it to the global ignore and/or exclude *.d.ts extensions from the lint commands somehow.

This is ready for review.

Why do you think that TS support for both @wordpress/eslint-plugin and @wordpress/scripts it's a breaking change rather than a new feature?

I'm not sure it is a breaking change, but I guess if someone had been using @wordpress/eslint-plugin and had already integrated TypeScript into their project, suddenly they'll start getting new errors to fix if they hadn't been linting TypeScript files. I think I'd classify that as a breaking change rather than just a new feature, at least if new features should be 'non-disruptive' this isn't that.

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:

• We're introducing new rules.
• We're linting files that wouldn't be linted before (*.ts).

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 personally don't have enough expertise to tell that everything is perfect, but it is good enough for me to start using it with Gutenberg.

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 next package releases? Maybe we can start getting feedback from other projects. @nerrad may have know some projects interested in using some of the TypeScript lint/prettier functionality.

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.

If we're blocked on package releases can we next package releases?

Sure thing, we can do it as soon as there is the next Gutenberg plugin RC ready 👍🏻

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 next package releases? Maybe we can start getting feedback from other projects. @nerrad may have know some projects interested in using some of the TypeScript lint/prettier functionality.

We're implementing TS in WooCommerce Blocks feature plugin and definitely could benefit from this update (which looks good afaict).