msupply-foundation / react-native-data-table

154 stars 35 forks source link

Full rewrite with VirtualisedList #46

Open Chris-Petty opened 5 years ago

Chris-Petty commented 5 years ago

Description

Key reasons:

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:

Cons:

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

Others:

Not doing

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