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

add a generic type for FormCreateOption #7119

Merged
merged 2 commits into from
Aug 9, 2017

Conversation

djyde
Copy link
Contributor

@djyde djyde commented Aug 7, 2017

By adding a generic type for FormCreateOption, we can now know what type the props in mapPropsToFields, onFieldsChange , onValuesChange is.

image

@mention-bot
Copy link

@djyde, thanks for your PR! By analyzing the history of the files in this pull request, we identified @simaQ, @benjycui and @RaoHai to be potential reviewers.

@afc163 afc163 requested review from yesmeck, infeng and BANG88 August 7, 2017 07:28
@codecov
Copy link

codecov bot commented Aug 7, 2017

Codecov Report

Merging #7119 into master will increase coverage by <.01%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #7119       /-   ##
==========================================
  Coverage   85.87%   85.87%    <.01%     
==========================================
  Files         231      231              
  Lines        4955     4957        2     
  Branches     1413     1415        2     
==========================================
  Hits         4255     4257        2     
  Misses        700      700
Impacted Files Coverage Δ
components/form/Form.tsx 89.09% <100%> ( 0.41%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 87bfe2d...a8306b5. Read the comment docs.

@BANG88
Copy link
Member

BANG88 commented Aug 7, 2017

LGTM.
But when you write options like below will have no effects:

const mapPropsToFields = (props: ArticleDetailOwnProps)  => props ...//
const onFieldsChange = (props, fields) => null

const ArticleDetailForm = Form.create<TOwnProps>({ onFieldsChange, mapPropsToFields })(ArticleDetail)

@djyde
Copy link
Contributor Author

djyde commented Aug 7, 2017

@BANG88 Of course, because when you define a function (or arrow function), the compiler has no idea where it will be, so the params is free to declare. But when you put the function to another function as a param (like Form.create), the compiler will check if the params type is correct.

@BANG88
Copy link
Member

BANG88 commented Aug 7, 2017

Maybe not, only useful when you write function inside options, otherwise we don't know the type definitions.

@djyde
Copy link
Contributor Author

djyde commented Aug 7, 2017

@BANG88

你可能没有理解我的意思,我用中文表达吧。

你在定义一个函数的时候,参数的类型肯定是自由的。但是当这个函数是用作参数的话,就是有限制的了。

比如:

const mapPropsToFields = (props) => {
  return {
  }
}

export default Form.create<ICommentFormProps>({
  mapPropsToFields
})(CommentForm)

这样编译器不会认为错,是因为 props 默认就是 any.

但是如果你传一个不是 ICommentFormProps(也就是 TOwnProps) 类型的时候,编译器就会报错:

image

所以如果你需要把函数在外定义,编译器只能保证这个函数作为参数传递时它接受的参数的类型是安全的 (除了any),至于 Intelisense, 就只能靠你手动声明类型了:

const mapPropsToFields = (props: TOwnProps) => {
  return {
  }
}

@BANG88
Copy link
Member

BANG88 commented Aug 7, 2017

嗯 定义在外面需要手动添加类型。

@afc163
Copy link
Member

afc163 commented Aug 9, 2017

conflict

@djyde
Copy link
Contributor Author

djyde commented Aug 9, 2017

resovled

@afc163 afc163 merged commit df93444 into ant-design:master Aug 9, 2017
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.

6 participants