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

[WIP]Tweak should component update #98

Closed
wants to merge 2 commits into from
Closed

[WIP]Tweak should component update #98

wants to merge 2 commits into from

Conversation

yesmeck
Copy link
Member

@yesmeck yesmeck commented Nov 4, 2016

发现一个问题,因为现在 columns 允许 render 是一个 function,这导致 TableRow 其实不是一个 pure component,不能用简单的 shouldComponentUpdate 去优化(例子:http://codepen.io/yesmeck/pen/oYNWwM?editors=001 )。我想后面再看看 Table 里的状态能不能像 currentHoverKey 那样抽到 store 里,然后把 shouldComponentUpdate 去掉,现在其实 shouldComponentUpdate 有点鸡肋。

@coveralls
Copy link

Coverage Status

Coverage increased (+0.6%) to 83.156% when pulling 37942a1 on yesmeck:tweak-shouldComponentUpdate into 179d4a5 on react-component:master.

@coveralls
Copy link

Coverage Status

Coverage increased (+1.4%) to 83.986% when pulling 6064324 on yesmeck:tweak-shouldComponentUpdate into 179d4a5 on react-component:master.

@yesmeck yesmeck changed the title Tweak should component update [WIP]Tweak should component update Nov 4, 2016
@yesmeck
Copy link
Member Author

yesmeck commented Nov 4, 2016

@benjycui 讨论了下,决定直接把 shouldComponentUpdate 去掉。

@afc163
Copy link
Member

afc163 commented Nov 4, 2016

这确实是个坑。

@afc163
Copy link
Member

afc163 commented Nov 7, 2016

https://github.com/react-component/table/blob/master/src/TableCell.jsx#L15-L17

TableCell 的 shouldComponentUpdate 貌似也同样问题。之前搜索 stateless function 和 pure component 的性能区别时,按 facebook/react#5677 (comment) 的说法貌似 stateless function 的性能都要更好,TableCell 里的优化也确实不一定有效。

@yesmeck
Copy link
Member Author

yesmeck commented Nov 7, 2016

TableCell 的 shouldComponentUpdate 确实没必要加,shouldComponentUpdate 也不是越多越好。

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.

3 participants