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

antd-mobile@2.0 file extension, type definition #1557

Closed
cncolder opened this issue Jul 11, 2017 · 13 comments
Closed

antd-mobile@2.0 file extension, type definition #1557

cncolder opened this issue Jul 11, 2017 · 13 comments
Assignees
Milestone

Comments

@cncolder
Copy link
Contributor

cncolder commented Jul 11, 2017

Updated by @paranoidjk

结论: #1566 (comment)

应该可以解决几个问题:

  • 去掉对 webpack.resolve 配置的依赖,统一收敛到 babel-plugin-import
  • web 用户使用更简单,服务端渲染不需要 require hack Add a server render demo with egg.js #1619
  • 类型文件可以分开维护,web & rn 的类型提示更准确
  • web 组件的用户不需要安装 react-native 依赖

What problem does this feature solve?

目前类型定义这块比较薄弱,组件只是提供了最基本的属性定义,有些缺失,有些 web 和 rn 混用。

很多类型定义上的错误都是在发布新版本之后由用户反馈再修补的。

What does the proposed API look like?

以下为建议,供参考:

类型定义并不要求面面俱到,只求能解决现有问题并逐步提高。

将现有的组件进行分类,源码风格类似的放在一个分类里,用统一的方法定义类型。

每种类型归纳出类型定义的最佳实践,然后套用。

新建 tests/types 文件夹,写一些 tsx 代码片段,然后用 tslint --type-check -p tests/types 检查错误,如果使用 vscode 编辑器甚至不必运行脚本直接打开文件就能看到错误提示。

// ./tests/types/web.tsx
import { Card, List } from '../../components'

const card = (
  <Card
    onClick={() => { }}
  />
)

const list = (
  <List
    onClick={() => { }}
  />
)
> tslint --type-check -p tests/types
Error at tests/types/web.tsx:11:5: Property 'onClick' does not exist on type 'IntrinsicAttributes & IntrinsicClassAttributes<List> & Readonly<{ children?: ReactNode; }> & Read...'.

发现类型问题就写一小段插入到这个文件里备用,之后再寻求解决办法。

类型定义错误低优先级,不加入 git hook,不加入 ci,提交代码可以暂时忽略类型问题。

@cncolder cncolder self-assigned this Jul 11, 2017
@cncolder
Copy link
Contributor Author

cncolder commented Jul 11, 2017

个人观点:过分追求 web 和 rn 类型统一可能无法完美实现。

List 为例:

web 版 List 根节点是 div,并且透传了其他属性给 div。而 div 拥有的属性是非常多的,仅仅事件就有几十个。

rn 版 List 根节点是 View,同样透传了其他属性,这些属性和 div 完全不同。

要想让 web List 在 typescript 层面透传其他属性,我们只能这样写:

class List extends React.Component<ListProps & React.HTMLAttributes<HTMLDivElement>, any>

要想拆分 web 和 rn 专有属性,可以这样写:

export interface ListProps {
  renderHeader?: Function | JSX.Element;
  renderFooter?: Function | JSX.Element;
  // children?: JSX.Element | JSX.Element[];
  // style?: React.CSSProperties | {} | Array<{}>;
  // role?: string;
}

export interface ListWebProps extends ListProps, React.HTMLAttributes<HTMLDivElement> {
  prefixCls?: string;
}

export default class List extends React.Component<ListWebProps, any> { }

@types/react 去维护 className/style/children 这些内置属性类型,先前我们自己指定的类型大部分是不正确的。

rn 版同理。

单纯看三个共用属性和两个特殊属性的话,5个属性不多,但实际上两边加起来有 100+ 属性混合在一起。

@cncolder
Copy link
Contributor Author

一份完整的 List type definition

import React from 'react';
import ReactNative from 'react-native'

interface ListProps {
  renderHeader?: Function | JSX.Element;
  renderFooter?: Function | JSX.Element;
}

export interface ListWebProps extends ListProps, React.HTMLAttributes<HTMLDivElement> {
  prefixCls?: string;
}

export interface ListNativeProps extends ListProps, ReactNative.ViewProperties {
  styles?: {
    Header?: ReactNative.TextStyle;
    Footer?: ReactNative.TextStyle;
    Body?: ReactNative.ViewStyle;
    BodyBottomLine?: ReactNative.ViewStyle;
  };
}

@paranoidjk
Copy link
Contributor

cc @bccsafe @warmhug

@cncolder
Copy link
Contributor Author

最终极的做法是删除 package.json typings 字段,单独维护并 push 到 DefinitelyTyped

DefinitelyTyped 允许多版本共存,比如 history history/v2 history/v3

我们可以效仿,antd-mobile antd-mobile/rn。再加上2.0计划中取消 .web.js 后缀,默认入口是 web 版,rn 用户可以引入包 import * from 'antd-mobile/rn

@paranoidjk
Copy link
Contributor

ant-design/antd-mobile-samples#21 这里的方案我觉得就够用了。push 到 DefinitelyTyped 维护难度会增加,分散,又没有 owner 权限

@cncolder
Copy link
Contributor Author

cncolder commented Jul 13, 2017

仔细查看 DefinitelyTyped 后发现他们是鼓励 Library 作者自己打包类型定义的,那里只是放一些作者不愿意打包的项目。

手工维护类型定义只是一个想法,对项目自身不利,不过要想提高代码里面的类型定义,难免要做许多改动,为了提高类型的精准度而引入 BUG 是我最担心的。我在慢慢熟悉 react-native,之后的修改我会尽量做到两边同时改。

@paranoidjk
Copy link
Contributor

paranoidjk commented Jul 14, 2017

结论: #1566 (comment)

应该可以解决两个问题:

  • 去掉对 webpack.resolve 配置的依赖,统一收敛到 babel-plugin-import
  • 类型文件可以分开维护,web & rn 的类型提示更准确
  • web 组件的用户不需要安装 react-native 依赖

antd-mobile@2.0 会改文件后缀 #1235

@cncolder 评估下改文件名后缀之后这套方案还有没有必要?

@cncolder
Copy link
Contributor Author

cncolder commented Jul 14, 2017

@paranoidjk 改文件名也就是 .web 和 .native 对调,配置工作从 web 用户转向 rn 用户,原有的问题依旧存在。

类型定义这块还要是弄的,而且我发现类型定义弄好了,可以更突出现有两套代码之间的差异,哪些差异需要抹平显得更直观。

@paranoidjk paranoidjk added this to the 2.0.0 milestone Jul 14, 2017
@paranoidjk
Copy link
Contributor

@cncolder 还有一个注意点

build出来的样式,如果没有 webpack.resolve 配置的话,babel-plugin-import 引用样式的时候要指定后缀,https://unpkg.com/antd-mobile@1.5.0-alpha.2/lib/button/style/

@cncolder
Copy link
Contributor Author

嗯,有空开另一个PR,和 #1566 一样,内部 style 和 component 一致,全部指向 index.web

@paranoidjk
Copy link
Contributor

2.x 改后缀为 .native.js, .js, 去掉对 webpack.reolve 依赖。 ssr 终于不需要 require hack 了。

https://github.com/ant-design/antd-mobile-samples/blob/master/web-ssr/next.config.js#L23

@paranoidjk
Copy link
Contributor

坑一个 require hacker 需要对应的文件必须首先存在,否则 nodejs 就 throw error 了

#1619

@paranoidjk paranoidjk changed the title [TS] Improve type definition and add test antd-mobile@2.0 file extension, type definition Jul 29, 2017
@paranoidjk paranoidjk mentioned this issue Jul 31, 2017
33 tasks
@paranoidjk
Copy link
Contributor

@cncolder #1632 已经 merge,你可以开始你的 尝试了。

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

No branches or pull requests

2 participants