react-native-data-table icon indicating copy to clipboard operation
react-native-data-table copied to clipboard

Full rewrite with VirtualisedList

Open Chris-Petty opened this issue 6 years ago • 0 comments

Description

Key reasons:

  • Performance bottleneck: Non-pure components causing excessive re-rendering. So everything must be PureComponents (or use React.memo())
  • ListView is deprecated from React Native. Realm ListView is tied to realm platform which is very annoying.

Expected behaviour/Goal

To be able to use the DataTable with 5,000 lines, 4 columns as efficiently and performant as with 1 line.

Context

Basically stolen from here https://github.com/openmsupply/mobile/issues/1043 . mSupply mobile causes heavy, unnecessary re-rendering of the entire table and its children when editing cells. This causes a big performance bottleneck with more than 100 rows (with say 5 columns).

Design Discussion/Solution

We're going to rewrite this package to use React-native VirtualisedList and define utility components (Rows, Cells...) with React.memo. There will be examples and guidance on optimal usage.

Are we keeping realm?

Decoupled. It's annoying for people wanting to use the table with anything other than realm. The coupling is Realm.ListView, but we're throwing that out with the rewrite anyway.

Are we using hooks? (This will require upgrading react-native, which should probably happen regardless, but will have to happen sooner rather than later. Keeping in mind the vaccine module branch is already a version behind)

Pros:

  • Latest API with concise notation
  • claimed to be less error prone and neater than the class API

Cons:

  • New API to learn
  • Reduces backwards compatibility

I say yes, though we're trading off backwards compatibility with older RN apps against using modern react APIs/style. The hooks API also has some new toys not included in the class API. Not that they are planning to deprecate the class API.

Doing both is gross. Lets commit to one, that is Hooks.

FlatList/VirtualizedList/Something else?

VirtualisedList allows a little more flexibility. We can fill in a few of props to make it as accessible as FlatList by default.

Component issues/discussions

  • [ ] DataTable #34
  • [ ] Row #40
  • [ ] Cell #37
  • [ ] EditableCell #38
  • [ ] TouchableCell #39
  • [ ] Header #41
  • [ ] HeaderCell #41
  • [ ] Custom cells

Others:

  • [ ] Testing #50
  • [ ] Styling #49
  • [ ] Debouncing on sorting/button pressing by default #6
  • [ ] Examples #1
  • [ ] Documentation #48
  • [ ] How to drill down a row (UX guy pls HALP!!!)
  • [ ] Horizontal scrolling
  • [ ] Anything to do with extraData prop...? (calculated fields? Row states/highlighting!?)
  • [ ] Is the dataDispatch prop easily given a redux dispatch? That'd be really nice.

Not doing

  • CheckableCell: Was stateful. Redundant with TouchableCell
  • TableButton: Redundant with TouchableCell
  • Expansions: They're a bit shit and will hurt some of the optimisations available. People can still still define their renderRow however they please, so it's still doable. Old implementation was stateful.
  • Sections: The props will be passed through to VirtualisedList, so the API will be there. People can try it but lets not design around it.

Performance metrics

Yes. TODO.

Tests

Yes. TODO.

Live examples

Written in RN. Should flex the whole API. Might tie into automated tests. CodePen or what ever is popular.

Additional notes

There have been requests for better support of reactXP and react-native-web (e.g. aria accessibility). I think that depending on what VirtualisedList is ported to in those frameworks, it could be naff using anything based on VirtualisedList including this package. Should implement native HTML for web based IMO.

More than ~40 TextInputs in table (i.e. Editable column) causes crashes https://github.com/facebook/react-native/issues/17530#issuecomment-416367184

Chris-Petty avatar Feb 13 '19 09:02 Chris-Petty