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

Jump to bottom

[Chore] Run ts-migrate on core package #286

Closed
wants to merge 8 commits into from
Closed

[Chore] Run ts-migrate on core package #286

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
7890066
Prevent no-used-vars lint error on type
chenesan Aug 21, 2020
6c329ea
Do ts-migrate on core package
chenesan Aug 24, 2020
70d7efb
Fix build error after ts-migrate on core
chenesan Aug 24, 2020
311b069
Upgrade eslint-plugin-react to 7.20.6 to fix eslint error after ts-mi…
chenesan Aug 24, 2020
1d1bd5c
Temporarily turn off some eslint rules for ts-migrate
chenesan Aug 24, 2020
4f81bb1
Add react/sort-comp lint rule fix after upgrade eslint-plugin-react
chenesan Aug 24, 2020
87acdb0
Resolve/Temp supress eslint error
chenesan Aug 24, 2020
aac5774
Fix test by adding propTypes temporarily
chenesan Aug 24, 2020
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
67 changes: 59 additions & 8 deletions configs/.eslintrc.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,18 +10,69 @@ plugins: [
'@typescript-eslint'
]

"rules":
"import/extensions": [
"error",
"ignorePackages",
{
rules:
"import/extensions": [
"error",
"ignorePackages",
{
"js": "never",
"jsx": "never",
"ts": "never",
"tsx": "never"
}
]

}
]
"no-unused-vars": "off"
"@typescript-eslint/no-unused-vars": ["error", { "ignoreRestSiblings": true }]
Comment on lines +24 to +25
Copy link
Contributor Author

Choose a reason for hiding this comment

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

這個是避免 import type 的時候造成 no-unused-vars error:
https://github.com/typescript-eslint/typescript-eslint/blob/master/packages/eslint-plugin/docs/rules/no-unused-vars.md
ignoreRestSiblings 是為了讓 const { a, b, ...other } = props 不會出錯，有蠻多地方我們會這樣寫來濾掉不要的 props。

# Temp turn off following rules because we're doing ts-migrate
# Will turn them on after migration completed.
"max-len": "warn"
"no-use-before-define": "warn"
Comment on lines +26 to +29
Copy link
Contributor Author

Choose a reason for hiding this comment

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

這就是在 ts-migrate 後暫時加入的 rule，理由是 ts-migrate 會加入太長的 @ts-expect-error comment，並且有產生一些會提前使用 type 的 code(違反 no-use-before-define)。

Copy link
Contributor

@benny0642 benny0642 Aug 26, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

稍微補充說明一下 @ts-expect-error 的用途：

The 3.9 release of TypeScript introduced @ts-expect-error comments. When a line is prefixed with a @ts-expect-error comment, TypeScript will suppress that error from being reported. If there’s no error, TypeScript will report that @ts-expect-error wasn’t necessary. In the Airbnb codebase we switched to using the @ts-expect-error comment instead of @ts-ignore.

簡單講標上 @ts-expect-error typescript 不會報錯，而且如果你修掉了，他還會提醒你要拿掉 @ts-expect-error。

ref: https://medium.com/airbnb-engineering/ts-migrate-a-tool-for-migrating-to-typescript-at-scale-cd23bfeb5cc

# It's fix after upgrading eslint-plugin-react to v7.20.6
# to keep consistency with rule config in eslint-config-ichef.
# just add `static-variables` in the first line.
# Should remove after we upgrade eslint-plugin-react in eslint-config-ichef.
'react/sort-comp': ['error', {
Comment on lines +30 to +34
Copy link
Contributor Author

Choose a reason for hiding this comment

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

這個是升了 eslint-plugin-react 到 7.20 後，有部分的 component 出現 react/sort-comp 的 error。
原因似乎是多了 static-variables 的 group，就會使一些本來不會出錯的地方出錯。
這裡把 eslint-config-ichef 的設定抄過來，加入 static-variables 來解決。
見 jsx-eslint/eslint-plugin-react#2408

Copy link
Contributor

@benny0642 benny0642 Aug 26, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

這個加在 eslint-config-ichef 中會不會比較好？因為在後台、online restaurant 都有用到 eslint-plugin-react，我們在共用的 eslint-config-ichef 修掉這個問題，就不用三個專案都寫類似的 rule 。

Copy link
Contributor Author

Choose a reason for hiding this comment

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

應該是要這樣，我想說暫時先讓它 pass，最後再來收尾
目前是想先盡快把 ts 加上 typing

Copy link
Contributor

Choose a reason for hiding this comment

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

可以標個 #TODO: 方便日後搜尋XD

Copy link
Contributor Author

Choose a reason for hiding this comment

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

感謝班尼建議，有另外開個 task 了

order: [
'static-variables',
'static-methods',
'instance-variables',
'lifecycle',
'getters',
'setters',
'/^(get|set)(?!(InitialState$|DefaultProps$|ChildContext$)).+$/',
'instance-methods',
'everything-else',
'/^(on|handle).+$/',
'rendering',
],
groups: {
lifecycle: [
'displayName',
'propTypes',
'contextTypes',
'childContextTypes',
'mixins',
'statics',
'defaultProps',
'constructor',
'getDefaultProps',
'getInitialState',
'state',
'getChildContext',
'componentWillMount',
'componentDidMount',
'componentWillReceiveProps',
'shouldComponentUpdate',
'componentWillUpdate',
'componentDidUpdate',
'componentWillUnmount',
],
rendering: [
'/^render.+$/',
'render',
],
},
}]

env:
browser: true
Expand Down
2 changes: 1 addition & 1 deletion package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -57,7 +57,7 @@
"eslint-import-resolver-babel-module": "^5.0.0-beta.1",
"eslint-plugin-import": "^2.22.0",
"eslint-plugin-jsx-a11y": "^6.1.2",
"eslint-plugin-react": "^7.11.1",
"eslint-plugin-react": "^7.20.6",
"extract-text-webpack-plugin": "^3.0.2",
"file-loader": "^1.1.6",
"fork-ts-checker-webpack-plugin": "^5.0.14",
Expand Down
2 changes: 2 additions & 0 deletions packages/core/configs/webpack.dist.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,8 @@ const defaultConfigs = require('../../../configs/webpack.dist');
const packageDirname = process.cwd();

module.exports = webpackMerge(defaultConfigs, {
entry: './src/index.ts',
Copy link
Contributor Author

Choose a reason for hiding this comment

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

對，因為現在是 index.ts 了。


module: {
rules: [
{
Expand Down
14 changes: 8 additions & 6 deletions packages/core/src/Avatar.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,12 +14,14 @@ const SQUARE = 'square';
const ROUNDED = 'rounded';
const CIRCLE = 'circle';

type AvatarPropTypes = {
className?: string,
src: string,
alt: string,
type: typeof SQUARE | typeof ROUNDED | typeof CIRCLE,
}
type OwnAvatarPropTypes = {
className?: string;
src: string;
alt: string;
type: typeof SQUARE | typeof ROUNDED | typeof CIRCLE;
};

type AvatarPropTypes = OwnAvatarPropTypes & typeof Avatar.defaultProps;

function Avatar({
className,
Expand Down
18 changes: 11 additions & 7 deletions packages/core/src/BasicRow.js → packages/core/src/BasicRow.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,19 +6,23 @@ import Tag from './Tag';

import wrapIfNotElement from './utils/wrapIfNotElement';

function BasicRow({
basic,
tag,
statusIcon,
children,
...otherProps
}) {
type OwnProps = {
basic?: React.ReactNode;
tag?: React.ReactNode;
statusIcon?: React.ReactNode;
};

type Props = OwnProps & typeof BasicRow.defaultProps;

// @ts-expect-error ts-migrate(2339) FIXME: Property 'children' does not exist on type 'Props'... Remove this comment to see the full error message
function BasicRow({ basic, tag, statusIcon, children, ...otherProps }: Props) {
if (!basic) {
return null;
}

return (
<div {...otherProps}>
{/* @ts-expect-error ts-migrate(2322) FIXME: Property 'children' does not exist on type 'Intrin... Remove this comment to see the full error message */}
<FlexCell shrink>{basic}</FlexCell>

{tag && wrapIfNotElement(tag, { with: Tag })}
Expand Down
29 changes: 15 additions & 14 deletions packages/core/src/Button.js → packages/core/src/Button.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,4 @@
import React from 'react';
import PropTypes from 'prop-types';
import classNames from 'classnames';

import icBEM from './utils/icBEM';
Expand All @@ -15,15 +14,22 @@ const RED = 'red';
const WHITE = 'white';
const BLACK = 'black';

const colors = [BLUE, RED, WHITE, BLACK] as const;

type OwnProps = {
color?: typeof colors[number],
solid?: boolean;
tagName?: 'button' | 'a' | 'div';
};

type Props = OwnProps & typeof Button.defaultProps;

function Button({
color,
solid,
tagName: ButtonTag,
color, solid, tagName: ButtonTag,
// React props
className,
children,
...otherProps
}) {
// @ts-expect-error ts-migrate(2339) FIXME: Property 'className' does not exist on type 'Props... Remove this comment to see the full error message
className, children, ...otherProps
}: Props) {
const bemClass = ROOT_BEM
.modifier(color)
.modifier('solid', solid);
Expand All @@ -38,12 +44,6 @@ function Button({
);
}

Button.propTypes = {
color: PropTypes.oneOf([BLUE, RED, WHITE, BLACK]),
solid: PropTypes.bool,
tagName: PropTypes.oneOf(['button', 'a', 'div']),
};

Button.defaultProps = {
color: BLACK,
solid: false,
Expand All @@ -53,6 +53,7 @@ Button.defaultProps = {
// export for tests
export { Button as PureButton };

// @ts-expect-error ts-migrate(4023) FIXME: Exported variable 'RowCompButton' has or is using ... Remove this comment to see the full error message
const RowCompButton = rowComp({ defaultMinified: true })(Button);
RowCompButton.defaultProps.bold = true;

Expand Down
36 changes: 19 additions & 17 deletions packages/core/src/Checkbox.js → packages/core/src/Checkbox.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,4 @@
import React, { PureComponent } from 'react';
import PropTypes from 'prop-types';
import classNames from 'classnames';
import prefixClass from './utils/prefixClass';
import icBEM from './utils/icBEM';
Expand All @@ -17,24 +16,22 @@ export const BEM = {
iconWrapper: ROOT_BEM.element('icon-wrapper'),
};

// @ts-expect-error ts-migrate(2322) FIXME: Property 'className' does not exist on type 'Intri... Remove this comment to see the full error message
export const CHECKBOX_BUTTON = <Icon type="empty" className={`${BEM.button}`} />;

class Checkbox extends PureComponent {
static propTypes = {
/**
* Use this to inject props to the underlying `<input />`
*/
input: PropTypes.object, // eslint-disable-line react/forbid-prop-types
indeterminate: PropTypes.bool,
overrideButton: PropTypes.element,

// <input type="checkbox" /> props
checked: PropTypes.bool,
defaultChecked: PropTypes.bool,
disabled: PropTypes.bool,
onChange: PropTypes.func,
};
type OwnProps = {
input?: any;
indeterminate?: boolean;
overrideButton?: React.ReactElement;
checked?: boolean;
defaultChecked?: boolean;
disabled?: boolean;
onChange?: (...args: any[]) => any;
};

type Props = OwnProps & typeof Checkbox.defaultProps;

class Checkbox extends PureComponent<Props> {
static defaultProps = {
input: {},
indeterminate: false,
Expand All @@ -46,11 +43,13 @@ class Checkbox extends PureComponent {
onChange: undefined
};

inputRef: any;

componentDidMount() {
this.updateInputIndeterminate();
}

componentDidUpdate(prevProps) {
componentDidUpdate(prevProps: Props) {
if (prevProps.indeterminate !== this.props.indeterminate) {
this.updateInputIndeterminate();
}
Expand All @@ -67,6 +66,7 @@ class Checkbox extends PureComponent {

renderCheckboxInput(inputProps, overrideButton) {
return (
// @ts-expect-error ts-migrate(2322) FIXME: Type 'BEMFactory' is not assignable to type 'strin... Remove this comment to see the full error message
<span className={BEM.iconWrapper}>
<input
ref={(ref) => { this.inputRef = ref; }}
Expand All @@ -90,6 +90,7 @@ class Checkbox extends PureComponent {
disabled,
onChange,
// React props
// @ts-expect-error ts-migrate(2339) FIXME: Property 'className' does not exist on type 'Reado... Remove this comment to see the full error message
className,
children,
...otherProps
Expand All @@ -114,5 +115,6 @@ class Checkbox extends PureComponent {
}
}

// @ts-expect-error ts-migrate(4082) FIXME: Default export of the module has or is using priva... Remove this comment to see the full error message
export default rowComp()(Checkbox);
export { Checkbox as PureCheckbox };
39 changes: 18 additions & 21 deletions packages/core/src/ColumnView.js → packages/core/src/ColumnView.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,4 @@
import React from 'react';
import PropTypes from 'prop-types';
import classNames from 'classnames';
import './styles/ColumnView.scss';

Expand All @@ -15,16 +14,26 @@ export const BEM = {
footer: ROOT_BEM.element('footer'),
};

type OwnProps = {
header?: React.ReactNode;
footer?: React.ReactNode;
flexBody?: boolean;
bodyPadding?: {
top?: number;
bottom?: number;
left?: number;
right?: number;
};
};

type Props = OwnProps & typeof ColumnView.defaultProps;

function ColumnView({
header,
footer,
flexBody,
bodyPadding,
header, footer, flexBody, bodyPadding,
// React props
className,
children,
...wrapperProps
}) {
// @ts-expect-error ts-migrate(2339) FIXME: Property 'className' does not exist on type 'Props... Remove this comment to see the full error message
className, children, ...wrapperProps
}: Props) {
const rootClassName = classNames(`${BEM.root}`, className);
const bodyClassName = BEM.body.modifier('flex', flexBody);

Expand Down Expand Up @@ -56,18 +65,6 @@ function ColumnView({
);
}

ColumnView.propTypes = {
header: PropTypes.node,
footer: PropTypes.node,
flexBody: PropTypes.bool,
bodyPadding: PropTypes.shape({
top: PropTypes.number,
bottom: PropTypes.number,
left: PropTypes.number,
right: PropTypes.number,
}),
};

ColumnView.defaultProps = {
header: undefined,
footer: undefined,
Expand Down
Loading