refactor: typescript implement#148
refactor: typescript implement#148dancerphil wants to merge 1 commit intootakustay:masterfrom dancerphil:master
Conversation
| node rollup.js | ||
| node node_modules/.bin/postcss -o style/index.css src/styles/index.css | ||
| node scripts/fix-css.js style/index.css | ||
| rm -rf cjs/ && tsc |
There was a problem hiding this comment.
恐怕我们不能现在就改成tsc编译。之前版本是一个bundle,它可以直接用于<script>引入等方式,而tsc的产物并不是打包的,这会是一个BREAKING
There was a problem hiding this comment.
在这个大版本中,乖乖保留原来的rollup构建吧,下个大版本改为tsc
There was a problem hiding this comment.
rollup 怎么生成 d.ts 啊
There was a problem hiding this comment.
版本控制可以交给你,我这个能发个 3.0-alpha.0 就好?
我想你后续对 3.0 也有很多构想吧。你可以在这个基础上搞。
There was a problem hiding this comment.
让tsc生成定义,然后用rollup生成bundle,这样行不?入口是index.js -> index.d.ts,能对上应该就没问题
There was a problem hiding this comment.
可能的问题
- dependencies 要不要加上,因为可能会依赖他们的类型
- import {xxx} from 'react-diff-view/es/tokenize' 会被 ts 支持,但是 js 不支持,因为类型好像必须是 esm 的,没法合起来,不过也许可以只覆盖一个 index.js,其他的还是 esm?不知道 script 引入的要求是什么?
| ); | ||
| } | ||
|
|
||
| // @ts-ignore |
There was a problem hiding this comment.
这是children的类型问题?这个类型能不能再强化一下,弄成tuple的话,jsx能不能认出来……
There was a problem hiding this comment.
应该只是 ts 类型推断上的问题,Children.count() 不会有推导,实际是对的,所以 ignore 了,不影响使用
| viewType, | ||
| gutterType = 'default', | ||
| selectedChanges = [], | ||
| widgets = {}, |
There was a problem hiding this comment.
这里个[]和{},最好放外面定义个常量?不然用默认值的时候,每次都是新对象,而react-diff-view整个性能非常依赖memo
| import {createContext, useContext} from 'react'; | ||
|
|
||
| interface Context { | ||
| gutterType: 'default' | 'none' | 'anchor'; |
There was a problem hiding this comment.
这几个string literal union感觉应该是单独的类型,用到的地方不少
| viewType: 'unified' | 'split'; | ||
| selectedChanges: string[]; | ||
| monotonous: boolean; | ||
| widgets: { |
There was a problem hiding this comment.
用Record<string, ReactNode>是不是比较好
| widgets: { | ||
| [key: string]: any; | ||
| }; | ||
| renderGutter: () => any; |
| type: 'insert'; | ||
| content: string; | ||
| isInsert: true; | ||
| isDelete?: boolean; |
There was a problem hiding this comment.
这里应该固定值,没有或者false吧,都知道是insert了
There was a problem hiding this comment.
这里严格会导致需要改代码,就不是初衷了
|
|
||
| export type Change = InsertChange | DeleteChange | NormalChange; | ||
|
|
||
| export interface HunkType { |
There was a problem hiding this comment.
这个类型的命名我有点想法,一般XxxType会给人是一种枚举的感觉,叫HunkInfo之类的会不会更好,下同
| isBinary: boolean; | ||
| // 'rename' 和 'copy' 理论上存在但实际很少用到 | ||
| type: 'add' | 'delete' | 'modify' | 'rename' | 'copy'; | ||
|
|
| generateAnchorID?: (change: Change) => any; | ||
| selectedChanges?: string[]; | ||
| tokens?: any[] | null; | ||
| widgets?: { |
There was a problem hiding this comment.
统一用Record吧,我记得我们的TS lint规则里有一条这个,只是react-diff-view现在没用我们的规则
| }; | ||
|
|
||
| const splitRangeToValidOnes = (hunks, start, end) => { | ||
| const splitRangeToValidOnes = (hunks: HunkType[], start: number, end: number): Array<[number, number]> => { |
There was a problem hiding this comment.
把[number, number]拉出来造一个Range的类型更有语义
There was a problem hiding this comment.
这个 PR 不会改语义,合入之后可以改。
There was a problem hiding this comment.
我的意思是,放一个type Range = [number number],在类型上更有可读性
|
This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions. |
|
Up!!! |
|
👀 |
|
I decide to move all implementation to TypeScript and publish version 2 with some small breaking changes, please track #189 for progress. |
Working in Progress
Do not merge.
This will let us provide
d.tsin nextmajororminorrelease. #135Something will include in this PR
siteSomething will not include in this but in next PR
recompose@testing-libary/react-hooksto add some tests