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

Table get stuck in loading state #612

Closed
alaingilbert opened this issue Mar 13, 2017 · 15 comments
Closed

Table get stuck in loading state #612

alaingilbert opened this issue Mar 13, 2017 · 15 comments
Labels

Comments

@alaingilbert
Copy link

Hello all,

I made a table with infinite scrolling.
In my scenario, I have many endpoints that returns empty lists ([]).

We start with 0 rows, and when to component first load, we set it to 1 row to trigger the infinite load.

  • We have one row "loading..."
  • Server is called, and returns [] (we set state to 0 rows) -> display "No rows" (so far so good)

Then we change some filters (click button) and want to call the server for new data.

  • We set the state to 1 row to trigger infinite scroll.
  • We now display "loading..."
  • But the "loadMore" functions is not called at this point, which make our app stock in loading state.

Here is a plunker to better explain my situation.
https://embed.plnkr.co/bgLtSvB8RB5gRhepWfRm/

Do you guys have any idea what I'm doing wrong ?
Thanks in advance.

@bvaughn
Copy link
Owner

bvaughn commented Mar 13, 2017

We set the state to 1 row to trigger infinite scroll.
We now display "loading..."
But the "loadMore" functions is not called at this point, which make our app stock in loading state.

Sounds like your hasRowLoaded function is not being cleared/reset correctly. InfiniteLoader is stateless. It will ask (and re-ask) if you've loaded the same row many times. It relies on your application to know when a row has been loaded (and to clear the cache when eg filters change).

In your example, you're never actually loading data. For example:

    return new Promise((resolve, reject) => {
      setTimeout(() => {
        var newDataFromServer = [];
        // This line will only ever assign 0 because `newDataFromServer` is always empty
        var newCount = newDataFromServer.length > 0 ? newDataFromServer.length + 1 : 0;
        this.setState({remoteRowCount: newCount});
        resolve(newDataFromServer);
      }, 250);
    });

Also, nothing is ever updating the list array even though some of your function props reference it.

Here's a fork of your Plnkr that works (in the way I imagine you wanting it to): https://plnkr.co/edit/lwiMkw?p=preview

@alaingilbert
Copy link
Author

alaingilbert commented Mar 13, 2017

Thanks for taking the time to review this.

Still, in my scenario, let say the button is actually a "filter" and will call a different endpoint on the server.
But, The server don't have any data [] for the endpoint I'm calling.
Which means, we want to override the list with the new data which is still empty [].
We want the state to be set to 0 rows again.

click the button set state to -> 1.
server says no data ---> 0 (set it back to 0)

@bvaughn
Copy link
Owner

bvaughn commented Mar 13, 2017

I'm not sure what your follow-up question is. 😄 What you have described is supported by react-virtualized. I suspect you're not setting/resetting some local state correctly in the case you're describing. It's hard for me to diagnose without seeing your application though.

@alaingilbert
Copy link
Author

I guess I just don't understand why LoadMoreRows is not called in this situation:

  • Constructor set RowCount = 0
  • ComponentDidMount set RowCount = 1
  • LoadMoreRows is called (great) -> set RowCount = 0
  • Click button, set RowCount = 1
  • Expect LoadMoreRows (but it's not called) <<---- bug ?

@bvaughn
Copy link
Owner

bvaughn commented Mar 13, 2017

Reproduce it on Plnkr.

@alaingilbert
Copy link
Author

alaingilbert commented Mar 13, 2017

https://embed.plnkr.co/bgLtSvB8RB5gRhepWfRm/
More or less the same as the previous one, I moved the list in the state.

@alaingilbert
Copy link
Author

alaingilbert commented Mar 13, 2017

Looks like I have to call it myself because of memoization:
(https://github.com/bvaughn/react-virtualized/blob/master/docs/InfiniteLoader.md#memoization-and-rowcount-changes)

  handleClick() {
    // We want to call the server again
    this.setState({remoteRowCount: 1}, () => {
      this.loadMoreRows({
        startIndex: 0,
        stopIndex: 1,
      })
    });
  }

Seems to work

Thanks again for the great lib :)

@bvaughn
Copy link
Owner

bvaughn commented Mar 14, 2017

👍 Glad you got things sorted out. You're welcome!

@alaingilbert
Copy link
Author

alaingilbert commented Mar 14, 2017

Hi, it's me again :(
It looks like the hack I found does not really work. (server get called twice sometime)

So I made a new plunker with a real world scenario.

Scenario

You have a search bar to search on the server.
When you search, the server returns corresponding data.

Steps to reproduce bug:

  • Search for banana. (server has no banana)
  • Then try to search anything else.
  • Page is stuck in loading mode because of memoization

Expected behavior:

  • Search for banana. (no rows)
  • Search other term -> page actually call loadMoreRows and get the rows.

Plunker

https://plnkr.co/edit/bgLtSvB8RB5gRhepWfRm?p=preview

Additional infos:

  • Same problem occur when the server returns only 1 row. (search for carrots)

PS: Looks like I have the same issue as this guy: #361 (comment)
But I don't understand how he fixed his issue

@bvaughn
Copy link
Owner

bvaughn commented Mar 14, 2017

I'd actually forgotten that I'd added memoization to that callback back with a4ac8a5. (I haven't used this component in a while.)

Some form of memoization is useful in order to prevent InfiniteLoader from re-asking for the same set of rows while data is in-progress loading. For example, your isRowLoaded callback just checks if index < list.length. This check would return false multiple times if a scroll event was triggered repeatedly (as they usually are) which would cause some apps (like yours) to fire many load requests. You could prevent this by tracking load-in-progress state as well but that would require additional complexity in application code. I'd like to avoid that potential gotcha.

However, I can see how the current implementation is problematic.

I could try to only cache while a request is in progress- except that I don't require a promise to be returned from loadMoreRows so that would be a backwards-breaking change.

So maybe the best thing to do is to provide an API to reset/clear the memoized cache.

Thoughts?

@alaingilbert
Copy link
Author

alaingilbert commented Mar 14, 2017

I think most infinite scroll libraries I used, implement a flag (infinite-scroll-disabled https://sroze.github.io/ngInfiniteScroll/documentation.html) to avoid multiple calls to the fetch function.
But this means that your component needs a state.

I think an easy solution would be to provide a public function to clear the memoizer.
So when a user wants to "reset" the search, he can just call this method to reset the InfiniteLoader.

@alaingilbert alaingilbert changed the title Table get stock in loading state Table get stuck in loading state Mar 14, 2017
@alaingilbert
Copy link
Author

alaingilbert commented Mar 15, 2017

I successfully implemented an easy fix which looks like this:

Edit: It seems that this fix is not working 100% of the time.
Reproduce bug with: Search for carrots than something else.


Edit2::
This actually fix it:

resetList() {
  this._child.resetMemoizer();
  this.setState({list: [], rowCount: 0}, () => {
    this.setState({rowCount: 1});
  });
}

Inside InfiniteLoader class:

resetMemoizer() {
  this._loadMoreRowsMemoizer = createCallbackMemoizer()
}

Then, in my code I have:

<InfiniteLoader ref={(child) => { this._child = child; }}

and:

resetList() {
  this._child.resetMemoizer()

Plunker

https://plnkr.co/edit/YXZtAAAswXsqD27DItTv?p=preview

@bvaughn
Copy link
Owner

bvaughn commented Mar 15, 2017

Thank you for the updates! I'll be happy to add a resetMemoizer method to InfiniteLoader to enable this use-case.

@bvaughn
Copy link
Owner

bvaughn commented Mar 15, 2017

9.3.0 was just released. InfiniteLoader has a new public method, resetLoadMoreRowsCache, that you can use along with the approach you described above. Thanks for digging in and helping with this. 😄

@alaingilbert
Copy link
Author

Awesome, that was fast.
Thank you again !

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