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

Scripts: Add TypeScript support to linting command #27143

Merged
merged 20 commits into from
Feb 25, 2021
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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

Large diffs are not rendered by default.

4 changes: 2 additions & 2 deletions package.json
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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

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