Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Sign up
Appearance settings

[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

Closed

Conversation

Copy link

@renekliment renekliment commented Jul 11, 2018
edited
Loading

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.

AllenFang reacted with thumbs up emoji
@renekliment renekliment changed the title (削除) [WIP] Row as a pure component (削除ここまで) (追記) [WIP] Row as a React.PureComponent (追記ここまで) Jul 11, 2018
Copy link
Author

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

Copy link
Member

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

Copy link
Member

AllenFang commented Jul 15, 2018
edited
Loading

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 👍

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

Copy link
Author

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

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?

Copy link
Author

@AllenFang Can't wait!

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
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

AltStyle によって変換されたページ (->オリジナル) /