Skip to content
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

Full rewrite with VirtualisedList #46

Open
17 tasks
Chris-Petty opened this issue Feb 13, 2019 · 0 comments
Open
17 tasks

Full rewrite with VirtualisedList #46

Chris-Petty opened this issue Feb 13, 2019 · 0 comments
Labels

Comments

@Chris-Petty
Copy link
Contributor

Chris-Petty commented Feb 13, 2019

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 msupply-foundation/mobile#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

Others:

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 facebook/react-native#17530 (comment)

@Chris-Petty Chris-Petty added Pure/Virtualised list rewrite Priority: normal Bug or feature but has work around and removed Priority: normal Bug or feature but has work around labels Jul 8, 2019
@Chris-Petty Chris-Petty changed the title Full rewrite Full rewrite with VirtualisedList Jul 23, 2019
Chris-Petty added a commit that referenced this issue Aug 6, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

2 participants