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.

Sign up for GitHub

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

Scripts: Add TypeScript support to linting command #27143

Merged
merged 20 commits into from
Feb 25, 2021
Merged

Scripts: Add TypeScript support to linting command #27143

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
20 commits
Select commit Hold shift + click to select a range
1b6d2cc
Add ts and tsx to lint-stages scripts
sirreal Nov 20, 2020
5572871
Add and use `@typescript-eslint/parser`
sirreal Nov 20, 2020
d98c1bd
Add plugin
sirreal Nov 24, 2020
2dfaec3
Add ts eslint-recommended
sirreal Nov 25, 2020
9a1b199
Demonstrate and fix some type issues
sirreal Feb 12, 2021
8df1ea2
Disable JSDoc type rules for TS
sirreal Feb 12, 2021
151cf38
Add changelog
sirreal Feb 12, 2021
94650a3
Clarify changelog
sirreal Feb 12, 2021
ae3a72c
Add js,jsx,ts,tsx extensions to lint-js
sirreal Feb 12, 2021
f2baa50
Add changelog
sirreal Feb 12, 2021
2a9d1e4
Fix comment
sirreal Feb 17, 2021
d2659f6
Allow eslint to resolve typescript files
sarayourfriend Feb 17, 2021
e613b0f
Fix auto-fixable errors in types.ts files
sarayourfriend Feb 17, 2021
03fe41c
Fix type imports
sarayourfriend Feb 19, 2021
c400bf0
Scope ts parser
sarayourfriend Feb 21, 2021
27f8ba2
Disallow types imported as values
sarayourfriend Feb 21, 2021
3013d31
Fix last remaining errors
sarayourfriend Feb 22, 2021
6fd8aa8
Use the correct parser for typescript but default to babel for the rest
sarayourfriend Feb 23, 2021
0c27d6b
Ignore *.d.ts files
sarayourfriend Feb 24, 2021
3057c4c
Remove unnecessary ignores
sarayourfriend Feb 25, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
449 changes: 449 additions & 0 deletions package-lock.json
View file
Open in desktop

Large diffs are not rendered by default.

4 changes: 2 additions & 2 deletions package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -292,14 +292,14 @@
"*.scss": [
"wp-scripts lint-style"
],
"*.js": [
"*.{js,ts,tsx}": [
"wp-scripts format-js",
"wp-scripts lint-js"
sirreal marked this conversation as resolved.
Show resolved Hide resolved
],
"{docs/{toc.json,tool/*.js},packages/{*/README.md,components/src/*/**/README.md}}": [
"node ./docs/tool/index.js"
],
"packages/**/*.js": [
"packages/**/*.{js,ts,tsx}": [
"node ./bin/api-docs/update-api-docs.js",
"node ./bin/api-docs/are-api-docs-unstaged.js",
"node ./bin/packages/lint-staged-typecheck.js"
Expand Down
6 changes: 5 additions & 1 deletion packages/components/package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -67,7 +67,11 @@
"tinycolor2": "^1.4.2",
"uuid": "^8.3.0"
},
"peerDependencies": {
"@wp-g2/create-styles": "^0.0.154",
"reakit-utils": "^0.15.1"
},
"publishConfig": {
"access": "public"
}
}
}
11 changes: 9 additions & 2 deletions packages/components/src/ui/control-group/types.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,13 @@
import { CSSProperties } from 'react';
/**
* External dependencies
*/
// eslint-disable-next-line no-restricted-imports
import type { CSSProperties } from 'react';

import { FlexProps } from '../flex/types';
/**
* Internal dependencies
*/
import type { FlexProps } from '../flex/types';

export type ControlGroupContext = {
isFirst?: boolean;
Expand Down
2 changes: 1 addition & 1 deletion packages/components/src/ui/control-label/types.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
/**
* Internal dependencies
*/
import { Props as TextProps } from '../text/types';
import type { Props as TextProps } from '../text/types';

export type Props = TextProps & {
isBlock?: boolean;
Expand Down
6 changes: 5 additions & 1 deletion packages/components/src/ui/elevation/types.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,8 @@
import { CSSProperties } from 'react';
/**
* External dependencies
*/
// eslint-disable-next-line no-restricted-imports
import type { CSSProperties } from 'react';

export type Props = {
/**
Expand Down
12 changes: 10 additions & 2 deletions packages/components/src/ui/flex/types.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,13 @@
import { CSSProperties } from 'react';
import { ResponsiveCSSValue } from '../utils/types';
/**
* External dependencies
*/
// eslint-disable-next-line no-restricted-imports
import type { CSSProperties } from 'react';

/**
* Internal dependencies
*/
import type { ResponsiveCSSValue } from '../utils/types';

export type FlexDirection = ResponsiveCSSValue<
CSSProperties[ 'flexDirection' ]
Expand Down
10 changes: 7 additions & 3 deletions packages/components/src/ui/form-group/types.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,10 +1,14 @@
import { CSSProperties, ReactNode, ReactText } from 'react';
/**
* External dependencies
*/
// eslint-disable-next-line no-restricted-imports
import type { CSSProperties, ReactNode, ReactText } from 'react';

/**
* Internal dependencies
*/
import { Props as ControlLabelProps } from '../control-label/types';
import { Props as GridProps } from '../grid/types';
import type { Props as ControlLabelProps } from '../control-label/types';
import type { Props as GridProps } from '../grid/types';

export type FormGroupLabelProps = ControlLabelProps & {
labelHidden?: boolean;
Expand Down
6 changes: 5 additions & 1 deletion packages/components/src/ui/grid/types.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,8 @@
import { CSSProperties } from 'react';
/**
* External dependencies
*/
// eslint-disable-next-line no-restricted-imports
import type { CSSProperties } from 'react';

type ResponsiveCSSValue< T > = Array< T | undefined > | T;

Expand Down
6 changes: 4 additions & 2 deletions packages/components/src/ui/h-stack/component.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@ import { createComponent } from '../utils';
import { useHStack } from './hook';

/**
*`HStack` (Horizontal Stack) arranges child elements in a horizontal line.
* `HStack` (Horizontal Stack) arranges child elements in a horizontal line.
*
* `HStack` can render anything inside.
*
Expand All @@ -30,8 +30,10 @@ import { useHStack } from './hook';
* }
* ```
*/
export default createComponent( {
const HStack = createComponent( {
as: 'div',
useHook: useHStack,
name: 'HStack',
} );

export default HStack;
12 changes: 10 additions & 2 deletions packages/components/src/ui/h-stack/types.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,13 @@
import { CSSProperties } from 'react';
import { FlexProps } from '../flex/types';
/**
* External dependencies
*/
// eslint-disable-next-line no-restricted-imports
import type { CSSProperties } from 'react';

/**
* Internal dependencies
*/
import type { FlexProps } from '../flex/types';

export type HStackAlignment =
| 'bottom'
Expand Down
8 changes: 8 additions & 0 deletions packages/components/src/ui/text/types.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,12 @@
/**
* Internal dependencies
*/
import type { Props as TruncateProps } from '../truncate/types';

/**
* External dependencies
*/
// eslint-disable-next-line no-restricted-imports
import type { CSSProperties } from 'react';

type TextAdjustLineHeightForInnerControls =
Expand Down
7 changes: 5 additions & 2 deletions packages/components/src/ui/utils/types.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,8 @@
import { As } from 'reakit-utils/types';
import { ViewOwnProps } from '@wp-g2/create-styles';
/**
* External dependencies
*/
import type { As } from 'reakit-utils/types';
import type { ViewOwnProps } from '@wp-g2/create-styles';

export type Options< T extends As, P extends ViewOwnProps< {}, T > > = {
as: T;
Expand Down
12 changes: 10 additions & 2 deletions packages/components/src/ui/v-stack/types.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,13 @@
import { CSSProperties } from 'react';
import { HStackAlignment, Props as HStackProps } from '../h-stack/types';
/**
* External dependencies
*/
// eslint-disable-next-line no-restricted-imports
import type { CSSProperties } from 'react';

/**
* Internal dependencies
*/
import type { HStackAlignment, Props as HStackProps } from '../h-stack/types';

export type Props = HStackProps & {
alignment?: HStackAlignment | CSSProperties[ 'alignItems' ];
Expand Down
4 changes: 4 additions & 0 deletions packages/eslint-plugin/CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,10 @@

## Unreleased

### Breaking Changes

- Add support and configuration for TypeScript files. [#27143](https://github.com/WordPress/gutenberg/pull/27143)

### New Features

- Enabled `import/default` and `import/named` rules in the `recommended` ruleset. [#28513](https://github.com/WordPress/gutenberg/pull/28513)
Expand Down
23 changes: 23 additions & 0 deletions packages/eslint-plugin/configs/recommended.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,12 +13,35 @@ const { config: localPrettierConfig } =
const prettierConfig = { ...defaultPrettierConfig, ...localPrettierConfig };

module.exports = {
settings: {
'import/resolver': {
gziolo marked this conversation as resolved.
Show resolved Hide resolved
node: {
extensions: [ '.js', '.jsx', '.ts', '.tsx' ],
},
},
'import/core-modules': [ 'react' ],
},
extends: [
require.resolve( './recommended-with-formatting.js' ),
'plugin:prettier/recommended',
'prettier/react',
'plugin:@typescript-eslint/eslint-recommended',
],
ignorePatterns: [ '**/*.d.ts' ],
rules: {
'prettier/prettier': [ 'error', prettierConfig ],
},
overrides: [
{
files: [ '**/*.ts', '**/*.tsx' ],
parser: '@typescript-eslint/parser',
rules: {
// 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',
},
},
],
};
2 changes: 2 additions & 0 deletions packages/eslint-plugin/package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,8 @@
],
"main": "index.js",
"dependencies": {
"@typescript-eslint/eslint-plugin": "^4.15.0",
"@typescript-eslint/parser": "^4.15.0",
"@wordpress/prettier-config": "file:../prettier-config",
"babel-eslint": "^10.1.0",
"cosmiconfig": "^7.0.0",
Expand Down
8 changes: 8 additions & 0 deletions packages/react-i18n/README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,14 @@ ReactDom.render(
You can also instantiate the provider without the `i18n` prop. In that case it will use the
default `I18n` instance exported from `@wordpress/i18n`.

_Parameters_

- _props_ ``: i18n provider props.

_Returns_

- ``: Children wrapped in the I18nProvider.

<a name="useI18n" href="#useI18n">#</a> **useI18n**

React hook providing access to i18n functions. It exposes the `__`, `_x`, `_n`, `_nx`,
Expand Down
18 changes: 14 additions & 4 deletions packages/react-i18n/src/index.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,3 +1,10 @@
/**
* External dependencies
*/
// 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';
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

@gziolo gziolo Feb 17, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe we should list React as peer dependency in that case? There are more packages that would require the same handling.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we should just disable those rules for packages/components... G2 imports a lot from reakit as well, outside of typescript files.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


/**
* WordPress dependencies
*/
Expand All @@ -8,9 +15,7 @@ import {
useMemo,
useReducer,
} from '@wordpress/element';
import { defaultI18n } from '@wordpress/i18n';
import type { I18n } from '@wordpress/i18n';
import type { ComponentType, PropsWithChildren } from 'react';
import { defaultI18n, I18n } from '@wordpress/i18n';
import type { Subtract } from 'utility-types';

interface I18nContextProps {
Expand All @@ -24,6 +29,8 @@ interface I18nContextProps {

/**
* Utility to make a new context value
*
* @param i18n
*/
function makeContextValue( i18n: I18n ): I18nContextProps {
return {
Expand Down Expand Up @@ -59,8 +66,11 @@ type I18nProviderProps = PropsWithChildren< { i18n: I18n } >;
*
* You can also instantiate the provider without the `i18n` prop. In that case it will use the
* default `I18n` instance exported from `@wordpress/i18n`.
*
* @param props i18n provider props.
* @return Children wrapped in the I18nProvider.
*/
export function I18nProvider( props: I18nProviderProps ) {
export function I18nProvider( props: I18nProviderProps ): JSX.Element {
const { children, i18n = defaultI18n } = props;
const [ update, forceUpdate ] = useReducer( () => [], [] );

Expand Down
3 changes: 3 additions & 0 deletions packages/scripts/CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,9 @@
### New Features

- Default `check-engines` command to the `engines` config in `package.json` file of the current project ([#29066](https://github.com/WordPress/gutenberg/pull/29066)).
### Breaking Changes

- Lint TypeScript files as part of `lint-js`. [#27143](https://github.com/WordPress/gutenberg/pull/27143)

### Enhancements

Expand Down
5 changes: 5 additions & 0 deletions packages/scripts/scripts/lint-js.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -47,11 +47,16 @@ const defaultIgnoreArgs = ! hasIgnoredFiles
? [ '--ignore-path', fromConfigRoot( '.eslintignore' ) ]
: [];

const defaultExtArgs = hasArgInCLI( '--ext' )
? []
: [ '--ext', 'js,jsx,ts,tsx' ];

const result = spawn(
resolveBin( 'eslint' ),
[
...defaultConfigArgs,
...defaultIgnoreArgs,
...defaultExtArgs,
...args,
...defaultFilesArgs,
],
Expand Down
1 change: 1 addition & 0 deletions tsconfig.base.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@
"noUnusedParameters": true,
"noImplicitReturns": true,
"noFallthroughCasesInSwitch": true,
"importsNotUsedAsValues": "error",

/* Module Resolution Options */
"moduleResolution": "node",
Expand Down