Skip to content

Conversation

@renekliment
Copy link

@renekliment renekliment commented Jul 11, 2018

So this is a first attempt to make this table library more performant. Partially addresses #392.
This PR covers only the Row component (in Body), because it is the most important and it covers only some cases. It handles rowClasses and rowEvents well (at least in our case), which is our use-case.

Current issues of this PR:

  1. It doesn't handle selectRow well when it has selected that changes. We probably need deep comparison there. Also, if someone changes selectRow.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.
  2. Not sure if creating the empty placeholders for all things is a good pattern. We could also do emptyPlainObject = {}; and emptyArray = []; and share that globally. Do we care?
  3. I've made an ugly thing with the cellSelectionInfo. Not sure if merg-eable like this.
  4. I had to make resolveSelectRowProps static for the purpose above. Also not sure if it makes sense mixed like this with the other methods.
  5. Putting stuff into 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.
  6. ADDED: It needs the very same data not to re-render. So we probably need some sort of deep comparison and internal state for data rows too? This should actually be responsibility of the caller I guess?
  7. Probably other stuff due to my inexperience with this library.

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.

@renekliment renekliment changed the title [WIP] Row as a pure component [WIP] Row as a React.PureComponent Jul 11, 2018
@renekliment
Copy link
Author

@AllenFang Could you please provide some pointers, both general and for the issues mentioned in the description? Thank you

@AllenFang
Copy link
Member

@renekliment yes, I'm already work on it.

@AllenFang
Copy link
Member

AllenFang commented Jul 15, 2018

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 👍

@AllenFang
Copy link
Member

if you agree my opinion, I think I can open a branch to do this with you and I can refactoring cell component firsly

@renekliment
Copy link
Author

@AllenFang Great, I'm all up for it :-)
Thank you.

@AllenFang
Copy link
Member

@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 AllenFang closed this Jul 18, 2018
@renekliment
Copy link
Author

@AllenFang Can't wait!

@AllenFang
Copy link
Member

@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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

2 participants