-
Notifications
You must be signed in to change notification settings - Fork 430
[WIP] Row as a React.PureComponent #404
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] Row as a React.PureComponent #404
Conversation
* so React.PureComponent can be used (since react 16)
@AllenFang Could you please provide some pointers, both general and for the issues mentioned in the description? Thank you
@renekliment yes, I'm already work on it.
Hello, firstly I'm very appreciated your contributions.
We are supposed to handle performance as well in this time. I agree some ideas from your description.
Actually, React.PureComponent
is good but it usually return false
when you compare an complex object,
You can put a componentWillRecieveProps
on the row.js
then compare the nextProps.selectRow
and this.props.selectRow
, it's always different. The best idea is we probably can solve this issue via immutable.js. But right now, I don't consider this solution, I mean immutable.js
.
In my opinion, we probably can consider to improve the performance from the cell level. i definitely know the current row design is too complex just like u said. However, cell is still clean and also a key point for performance, but before do it, we probably still need to do few refactoring so that make the cell component can accept primitive value. In addition , like customization part(https://github.com/react-bootstrap-table/react-bootstrap-table2/blob/master/packages/react-bootstrap-table2/src/cell.js#L65), I should move this out from cell to row so that we can compare the final value instead of function. which mean those custom function should be called on row component. After few refactoring, we can start to enhance the cell level performance firstly due to everything is simple than before.
About the row component, I think row is muck like a tiny container, it really hard to tweak, I will need to come out some solution and also don't rule out the possibility of a introducing new row components or container or HoC.
Please do let me know your thought. @renekliment 👍
if you agree my opinion, I think I can open a branch to do this with you and I can refactoring cell component firsly
@AllenFang Great, I'm all up for it :-)
Thank you.
@renekliment 👍
because there's a big change in the near future: #333
So after finish this, I will open a PR for working on the thing I mention above and let u know.
Close this PR and do u still want to work on the cell performance tweak?
@AllenFang Can't wait!
@renekliment 1.0.0
was release and I also start working on Cell
level component tuning. #449
After #449, I will start work on Row level
Uh oh!
There was an error while loading. Please reload this page.
So this is a first attempt to make this table library more performant. Partially addresses #392.
This PR covers only the
Row
component (inBody
), because it is the most important and it covers only some cases. It handlesrowClasses
androwEvents
well (at least in our case), which is our use-case.Current issues of this PR:
selectRow
well when it hasselected
that changes. We probably need deep comparison there. Also, if someone changesselectRow.selected
, it gets passed to every row, so that would need rework as well. Since this little thing is quite complex, it would probably be better to resolve this in another PR.empty
placeholders for all things is a good pattern. We could also doemptyPlainObject = {};
andemptyArray = [];
and share that globally. Do we care?cellSelectionInfo
. Not sure if merg-eable like this.resolveSelectRowProps
static for the purpose above. Also not sure if it makes sense mixed like this with the other methods.componentWillReceiveProps
is probably not a good idea, because it is deprecated and will be removed in React 17. I couldn't figure out another easy approach though.getDerivedStateFromProps
is static unfortunately.(削除) ADDED: It needs the very sameThis should actually be responsibility of the caller I guess?data
not to re-render. So we probably need some sort of deep comparison and internal state for data rows too? (削除ここまで)This is mainly to kick-off the discussion about this. I think it would be awesome, if even a basic version of this concept would be merged, since preventing some parts of the table to re-render needlessly is a much more difficult task and every small step is a benefit (especially doing this for the
Row
component).I appreciate ideas / comments / ...
Thank you.