Efficient React ListView for Realm collections #199

appden · 2016-01-12T00:13:59Z

This component is fully backwards compatible with the original React ListView, but is compatible with Realm Results and List objects to use their snapshot functionality along with more efficiently checking if each row should update.

This resolves #142 and #36

appden · 2016-01-12T15:34:27Z

retest this please

alazier · 2016-01-12T23:51:31Z

examples/ReactExample/components/todo-listview.js

 const TodoListItem = require('./todo-list-item');
 const realm = require('./realm');
 const styles = require('./styles');

-const { ListView, Text, View } = React;
+const { Text, View } = React;
+const { ListView } = RealmReact;


Should we consider naming it RealmListView to differentiate? This is fine if it is the convention but may be confusing.

No because it's supposed to be a drop-in replacement and it's namespace under the RealmReact module. So it's really called RealmReact.ListView.

alazier · 2016-01-12T23:55:15Z

Have we done any testing to see how this effects performance? Would be good to know if this resolves any of the issues reported.

We should also probably write tests for this.

appden · 2016-01-13T00:03:31Z

I have a separate test app where I fine tuned this to work with a reasonably large dataset (100,000 items). It removed all the JS bottlenecks, but there's definitely a performance issue with using large sorted results (inside core) that I will dive more into when I address #193.

I don't think the overhead of creating UI tests for this will be worth the effort at this point.

kristiandupont · 2016-01-13T13:12:34Z

examples/ReactExample/components/todo-listview.js

        });

-        this.state = {};
+        this.state = {
+            dataSource: this._cloneDataSource(dataSource, props),


I haven't been following the evolution completely so maybe I am missing something, but it seems odd to me that a data source would be state rather than a property or just a variable.

It is a bit weird at first, but since a dataSource is completely immutable it makes sense because the canonical way to update it is to clone it and call setState. Otherwise you'd need to call forceUpdate, which is kind of an anti-pattern in React.

alazier · 2016-01-14T22:47:29Z

Playing around with this a bit it looks like we have some perf issues. I added this code to the TodoApp constructor to populate the initial list with 1000 items:

                let list = realm.create('TodoList', {name: 'Todo List', items: []});
                for (var i = 0; i < 1000; i++) {
                    list.items.push({text: '' + i});
                };

After doing this scrolling performance is pretty bad, and there is a noticeable lag when mutating the list by removing items. The lag increases to further you scroll down - once you get down to around 300 items the ui takes about a second to update. It would be nice to know where this lag is coming from and to eliminate it if possible.

appden · 2016-01-15T01:46:58Z

I had to dive in pretty deep with the JavaScriptCore profiler to understand what's going here. This delay is overwhelmingly caused by React validating props, which is only enabled on debug builds. When built in Release mode, this problem should go away. However, we don't have the necessary build phase in the example app to bundle the release version of the JS (I will update this branch with that phase). You can get the release version of the JS by appending &dev=false to the bundle URL in AppDelegate.m, and you will see that there is no more delay when deleting todo items. 😄

appden · 2016-01-15T10:05:42Z

I opened PR #202 with updates to the way the example app chooses its JS.

alazier · 2016-01-19T00:34:31Z

I think this is fine other than the fact that we don't have tests verifying that this works. It is likely that at some point react-native will change and this will break. I think at a minimum we should add ui tests to the example app to make sure list items actually show up using our ListView. Since this is something we have been meaning to do anyway it may make sense to make it happen with this pr.

appden · 2016-01-21T06:47:36Z

Ok, I'll try to tackle a basic version of that for this PR.

This would unfortunately result in adverse performance side effects because calling size() can be expensive.

This component is fully backwards compatible with the original React ListView, but is compatible with Realm Results and List objects to use their snapshot functionality along with more efficiently checking if each row should update.

It's also been updated to follow best practices with how to properly use the ListView data source.

This caused an error with NPM v3 and was not necessary for us to have.

* master: (207 commits) Convert to using new React Native MainActivity template Create RealmReactPackage for our Android plugin Fix doc for Android NPM ignore react-native/android folder Skip building Android module under Xcode Skip building Android module for iOS tests gitignore Android downloads folder Update README with instructions to run on Android Use un-patched RN for Android by installing hook into JSC cleanup build system Use Realm in node_modules for ReactTests on Android Change Demo => ReactTests adding a 'publishAndroid' task to generate the AAR with prebuilt .so files Adjust POM_NAME Use consistent package naming Remove old Demo files Add copyright to JNI file Cleanup platform.hpp Make our Android module buildable as a dependency Add `npm test` command ...

These changes include timeouts when waiting on notifications.

Re-using the existing React testing class to hook into the example app. Right now, the test is extremely basic but is able to be expanded later. Resolves #36

appden · 2016-02-16T10:22:56Z

@alazier this is ready for review. It includes the ListView implementation that you reviewed before, and lots of updates to test that the ReactExample app properly loads as well as new unit tests for ListViewDataSource inside the ReactTests app.

Efficient React ListView for Realm collections

alazier reviewed Jan 12, 2016
View reviewed changes

kristiandupont reviewed Jan 13, 2016
View reviewed changes

appden added 13 commits January 21, 2016 11:41

Only call size() when actually needed

58ef90d

This would unfortunately result in adverse performance side effects because calling size() can be expensive.

Efficient React ListView for Realm collections

eb4ac00

This component is fully backwards compatible with the original React ListView, but is compatible with Realm Results and List objects to use their snapshot functionality along with more efficiently checking if each row should update.

Update ReactExample to use our new ListView

5772717

It's also been updated to follow best practices with how to properly use the ListView data source.

Delegate a couple ListView methods

7f25ed7

Update ReactTests Xcode project

55e67d9

Remove realm dependency in realm-tests

31db79e

This caused an error with NPM v3 and was not necessary for us to have.

Add and tweak ListView wrapper methods

a55faef

Don't build JS bundles for the simulator

23a7c5b

Make several improvements to how tests run under RN

1f030a0

These changes include timeouts when waiting on notifications.

Add test for ListViewDataSource to React Test App

0188517

Convert Example app to use ES6 import syntax

2bdb6bf

Test that example app loads properly

c64524f

Re-using the existing React testing class to hook into the example app. Right now, the test is extremely basic but is able to be expanded later. Resolves #36

appden force-pushed the sk-listview branch from 11067db to c64524f Compare February 15, 2016 23:47

appden added 3 commits February 15, 2016 16:56

Make ListViewDataSource test go quicker

5f9f00e

Increase React test timeout interval for CI

137e4d6

Update test script to actually test ReactExample

c01c0fa

alazier added a commit that referenced this pull request Feb 17, 2016

Merge pull request #199 from realm/sk-listview

d9abcf0

Efficient React ListView for Realm collections

alazier merged commit d9abcf0 into master Feb 17, 2016

appden deleted the sk-listview branch February 19, 2016 00:31

snyk-bot mentioned this pull request Sep 10, 2021

[Snyk] Upgrade url-parse from 1.4.3 to 1.5.3 BettrFinance/realm-js#4

Open

sync-by-unito bot mentioned this pull request Mar 15, 2024

Chaining multiple filtered queries at once crashes on Android #2637

Closed

github-actions bot locked as resolved and limited conversation to collaborators Mar 17, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Efficient React ListView for Realm collections #199

Efficient React ListView for Realm collections #199

appden commented Jan 12, 2016

appden commented Jan 12, 2016

alazier Jan 12, 2016

appden Jan 13, 2016

alazier commented Jan 12, 2016

appden commented Jan 13, 2016

kristiandupont Jan 13, 2016

appden Jan 13, 2016

alazier commented Jan 14, 2016

appden commented Jan 15, 2016

appden commented Jan 15, 2016

alazier commented Jan 19, 2016

appden commented Jan 21, 2016

appden commented Feb 16, 2016

Efficient React ListView for Realm collections #199

Efficient React ListView for Realm collections #199

Conversation

appden commented Jan 12, 2016

appden commented Jan 12, 2016

alazier Jan 12, 2016

Choose a reason for hiding this comment

appden Jan 13, 2016

Choose a reason for hiding this comment

alazier commented Jan 12, 2016

appden commented Jan 13, 2016

kristiandupont Jan 13, 2016

Choose a reason for hiding this comment

appden Jan 13, 2016

Choose a reason for hiding this comment

alazier commented Jan 14, 2016

appden commented Jan 15, 2016

appden commented Jan 15, 2016

alazier commented Jan 19, 2016

appden commented Jan 21, 2016

appden commented Feb 16, 2016