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

refactor: next in TypeScript #206

Merged
merged 10 commits into from
Aug 6, 2019
Merged

Conversation

monkindey
Copy link
Contributor

@monkindey monkindey commented Jul 28, 2019

主要涉及到 next 包的 TypeScript 重构,并且把一些 ant 包通用的 interface 抽出来。

@@ -3,7 +3,7 @@ import { registerFormField, createArrayField } from '@uform/react'
import { Icon } from 'antd'
import styled, { css } from 'styled-components'

export const CircleButton = styled.div.attrs({ className: 'cricle-btn' })`
export const CircleButton = styled['div'].attrs({ className: 'cricle-btn' })`
Copy link
Contributor Author

@monkindey monkindey Jul 28, 2019

Choose a reason for hiding this comment

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

很 hack 地改成bracket notation,不然 npm run start 会报错。我也不知道为啥。大家有其他解法么?

Copy link
Collaborator

Choose a reason for hiding this comment

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

试下加上 @types/styled-components 类型声明依赖?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

😂 加过了,又是一大丢报错,涉及到改动量略大,而且后面考虑干掉 styled-components,虽然是很强大的 css-in-JS 方案,但是用在底层组件感觉大材小用了。问题是在 npm run build 没问题,但是 npm run start 就有问题。不知道是不是 ts-loader 做了什么处理。

Copy link
Collaborator

Choose a reason for hiding this comment

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

styled-components短期内不会直接干掉,直接干掉的话,会有break change.最好先保证0.x的体验是没问题的,所以0.x还是得依赖@types/styled-components

Copy link
Contributor Author

@monkindey monkindey Jul 29, 2019

Choose a reason for hiding this comment

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

Break Change 是指哪一块

Copy link
Contributor

Choose a reason for hiding this comment

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

试下加上 @types/styled-components 类型声明依赖?

这个我也尝试过,感觉报错有点神奇。。。。我自己用emotion倒没这个问题。

Copy link
Collaborator

Choose a reason for hiding this comment

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

个人觉得,样式覆盖跟类名变动这个应该还好吧?渐进式迁移,并不需要一下子弄完它;引入样式的方式可以参考下 antd, 开发者自己可以完整地载入打包好的 css 文件,也可以按需加载对应的 css/less/scss/styl 文件吧?

这些变动感觉肯定会影响现有项目的,项目配置可能都需要改的

Copy link
Contributor Author

@monkindey monkindey Aug 4, 2019

Choose a reason for hiding this comment

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

引入 @types/styled-component 还会导致 npm run build 失败。DefinitelyTyped/DefinitelyTyped#33311
DefinitelyTyped/DefinitelyTyped#34882 两个 ISSUE 现在还 Open。我觉得咱们引入这个 @types/styled-component 成本太大了。而且最近规划 styled-component 是会被替代的,所以我觉得还是保留现在的写法吧。

大家意见如何? @janryWang @anyuxuan @atzcl @stkevintan

Copy link
Collaborator

Choose a reason for hiding this comment

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

目前的问题是包了styled-components的组件智能提示都失效了,有没有办法我们自己给它写类型定义,先保证智能提示不失效就行

Copy link
Collaborator

Choose a reason for hiding this comment

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

按照目前的规划,保持现有,不引入会比较好点吧

@@ -90,7 +109,7 @@ const Table = styled(
getColumns(children) {
const columns = []
React.Children.forEach(children, child => {
if (React.isValidElement(child)) {
if (React.isValidElement(child as React.ReactElement)) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

不做这个 as React.ReactElement 的话下面的 displayName 会报错。

Copy link
Collaborator

Choose a reason for hiding this comment

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

这里改成

...
const columns = [] as IColumnProps[]
React.Children.forEach<React.ReactElement<IColumnProps, typeof Column>>(...)
...

会不会好点?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OK

@@ -171,6 +177,7 @@ registerFormField(
}
if (listType.indexOf('dragger') > -1) {
return (
// @ts-ignore 感觉是 next 那边对于 Dragger 的 props 定义错了
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-ignore 是因为好像那边 next 对于 Dragger 的 props 定义错了。导致给它 value 或者其他 props 它都会报错。我后面去 next 提 Issue,看看是不是他们的问题。

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@monkindey monkindey requested a review from janryWang July 28, 2019 17:17
@monkindey monkindey assigned stkevintan and unassigned stkevintan Jul 28, 2019
@monkindey monkindey requested a review from stkevintan July 28, 2019 17:17
className?: string

// TODO
fixedWidth?: string | number
style?: object
Copy link
Collaborator

Choose a reason for hiding this comment

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

这里的 style 类型应该是 React.CSSProperties ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

好的。Good Catch

component?: any
children?: React.ReactNode
xxs?: ColSpanType | ColSize
Copy link
Collaborator

Choose a reason for hiding this comment

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

这里有大量的相同类型,感觉是不是可以定义一个类型来拓展呢?
比如:

type unionGridExpand<K extends keyof any> = {
  [P in K]?: ColSpanType | ColSize
}

export interface IColProps extends ColProps, unionGridExpand<'xxs' | 'xs' | 's' | 'm' | 'l' | 'xl'> {
  ....
}

不过这样可读性会比较差😅😅

Copy link
Contributor Author

Choose a reason for hiding this comment

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

😂 我看 antd 也是现在这样子写,所以我就按照它的来了。不过我可以看下你这种方式。

Copy link
Contributor Author

Choose a reason for hiding this comment

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

好建议,不过想了想还是保留声明式方式,可读性高点。

SMALL = 'small'
}

export interface IFormConsumerProps {
Copy link
Collaborator

Choose a reason for hiding this comment

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

可以把下方的 IFormPropsIFormItemProps 中的 className style 等等都拥有的属性提到 IFormConsumerProps 来?另外 style 也应该是 React.CSSProperties 吧?

fixedWidth?: string | number
style?: object
component?: any
Copy link
Collaborator

Choose a reason for hiding this comment

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

看了源码,这里的跟下面 IFormPropscomponent 是不是可以定义为 keyof JSX.IntrinsicElements | React.ComponentType<any>

Copy link
Contributor Author

Choose a reason for hiding this comment

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

JSX.IntrinsicElements 里面 input等一些标签好像不能有 children 吧?不过比 component?: any 好。

Copy link
Collaborator

Choose a reason for hiding this comment

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

那可能直接定一些常用并且允许拥有 children 的 html 元素会好点,实在不行可以传入一个泛型来联合声明,然后让调用方拓展 😅😅

Copy link
Contributor Author

Choose a reason for hiding this comment

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

先按你最早的方式来哈。

@monkindey monkindey merged commit 33e4bfb into alibaba:master Aug 6, 2019
@monkindey monkindey deleted the next-in-ts branch August 6, 2019 04:56
ZirkleTsing pushed a commit that referenced this pull request May 14, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants