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

feat: use ReadonlyArray in props #574

Merged
merged 1 commit into from Jan 27, 2021

Conversation

xiaoxiangmoe
Copy link
Contributor

@xiaoxiangmoe xiaoxiangmoe commented Jan 26, 2021

Our data types are immutable and we can't use rc-table unless we use .slice() to clone data and convert it to mutable array.

<Table data={myImmutableArray.slice()} />

But the implementation of rc-table does not rely on mutable array, it should be immutable.

So I change the props types of rc-table to immutable array such that I can use

<Table data={myImmutableArray} />

related issue: NG-ZORRO/ng-zorro-antd#6156 (comment)

@vercel
Copy link

vercel bot commented Jan 26, 2021

This pull request is being automatically deployed with Vercel (learn more).
To see the status of your deployment, click below or on the icon next to each commit.

🔍 Inspect: https://vercel.com/react-component/table/3r738ytgi
✅ Preview: https://table-git-fork-xiaoxiangmoe-master.react-component.now.sh

@codecov
Copy link

codecov bot commented Jan 26, 2021

Codecov Report

Merging #574 (1b7b4d6) into master (4cf7ec0) will not change coverage.
The diff coverage is 100.00%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #574   +/-   ##
=======================================
  Coverage   99.87%   99.87%           
=======================================
  Files          26       26           
  Lines         803      803           
  Branches      224      234   +10     
=======================================
  Hits          802      802           
  Misses          1        1           
Impacted Files Coverage Δ
src/Body/index.tsx 100.00% <ø> (ø)
src/ColGroup.tsx 100.00% <ø> (ø)
src/Header/Header.tsx 100.00% <ø> (ø)
src/Header/HeaderRow.tsx 100.00% <ø> (ø)
src/context/BodyContext.tsx 100.00% <ø> (ø)
src/context/TableContext.tsx 100.00% <ø> (ø)
src/utils/fixUtil.ts 100.00% <ø> (ø)
src/Body/BodyRow.tsx 100.00% <100.00%> (ø)
src/Header/FixedHeader.tsx 100.00% <100.00%> (ø)
src/Table.tsx 100.00% <100.00%> (ø)
... and 3 more

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 4cf7ec0...1b7b4d6. Read the comment docs.

@zombieJ
Copy link
Member

zombieJ commented Jan 27, 2021

CI failed. use readonly T[] instead.

@zombieJ
Copy link
Member

zombieJ commented Jan 27, 2021

还有 2 个 error 哈~

@xiaoxiangmoe
Copy link
Contributor Author

@zombieJ fixed

if (arr === undefined || arr === null) {
return [];
}

// @ts-expect-error
Copy link
Member

Choose a reason for hiding this comment

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

不要用 @ts-expect-error,其实只要处理入口的 readonly 就够了,内部很多 param 没有必要改成 readOnly 的

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@zombieJ 内部的也需要一起改,否则就需要进行强制类型转换了。

(顺便一提,这个地方没法写得类型安全,在 ts 4.1.2 是可以写成 function toArray<T>(arr: T | readonly T[]): readonly T[]通过编译的,但是 4.1.3 undo 了这个 PR)

microsoft/TypeScript#39258

Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Member

Choose a reason for hiding this comment

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

其实我觉得入口强转一下也 ok,内部其实也不是特别在乎。那就覆盖一下定义吧

@zombieJ
Copy link
Member

zombieJ commented Jan 27, 2021

+ rc-table@7.12.4

@xiaoxiangmoe
Copy link
Contributor Author

@zombieJ 我提一下 antd 的 PR

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.

None yet

2 participants