facebookarchive / fixed-data-table

A React table component designed to allow presenting thousands of rows of data.
http://facebook.github.io/fixed-data-table/
Other
4.3k stars 553 forks source link

Stateful cells losing state on scroll #200

Open sidnair opened 9 years ago

sidnair commented 9 years ago

Hi,

I'm running into an issue where the cell state gets reset after being scrolled out of and back into view. It looks like the cell gets recreated, causing it to lose its state. I'm passing a key prop, per https://github.com/facebook/fixed-data-table/pull/72, but that doesn't solve the issue.

Here's a simple example: http://jsbin.com/waceqavaco/edit?html,js,output. The table keeps track of how many times a cell has been clicked. To repro: 1) Click the top cell a few times, and observe that the number goes up 2) Scroll to the bottom the table 3) Scroll to the top. Observe that the count of the top cell is now 0

Do you know how to avoid this?

ehzhang commented 9 years ago

Hi! Fixed data table works by virtualizing your cells and only rendering what is in view. The cells themselves are rendered based off of data that is provided via the rowGetter, and individual cell state is not preserved as scrolling will recreate the cell as necessary.

At the your level, you will need to actually change the data source that backs the cell in question.

sidnair commented 9 years ago

Thanks for the fast response!

I made an updated snippet that passes in a callback via props to sets the state in the parent on click. http://jsbin.com/horijo/1/edit?html,js,output

Is that the idiomatic way to do it, or is there a better pattern?

ehzhang commented 9 years ago

This largely depends on how you choose to feed in your data!

We try to accommodate for the instance where the data is actually fed in through an outside source, and the cells act to manipulate that data source. In this case, you're using state of the parent, which is all good. Other implementations may choose to manipulate data in a flux store, for example.

So basically, what you have seems to be a perfectly reasonable way of doing this :)

sidnair commented 9 years ago

Awesome. Thanks so much for the help!

It might be worth adding an example with stateful cells to the docs if this use case is common. For context, I ran into this when working on a change to enable inline edit.

ehzhang commented 9 years ago

No problem!

We're actually focusing on changing the API right now, and the examples should follow.

sidnair commented 9 years ago

I was thinking about this a bit more, and it seems like the above solution breaks encapsulation. This is unfortunate when using my own components, but more importantly means I can't use any stateful third-party React components.

Is there a more general solution to this problem than editing the components I use to not use state?

One idea I had was to create a wrapper component for a cell that would recursively save state on unmount and restore it when the component is recreated, but I'm not sure if that would work or what that code would look like.

Also, do the API changes you mentioned for v0.5.0 have any implications for this use case?

ehzhang commented 9 years ago

Unfortunately, because this is a unique use case where we're virtualizing cells, we don't currently have a best general solution :(

There is a consideration that we should be saving state somewhere, but this isn't a use case that React works best with, and we can't really easily restore that state. Right now, the best option is to use props to render information properly using props.

I'm currently working to see if this is something that we can fix for the next version though.

sidnair commented 9 years ago

Would it make sense to have an option to turn off virtualization for cases where the table isn't huge and you want stateful cells? Do you have a sense of how large a table needs to be for virtualization to noticeably improve performance?

Glad to hear that you're looking into a fix for 0.5! Going to reopen this issue to reflect that, but feel free to close if that's not how you manage issues.

ehzhang commented 9 years ago

@sidnair while turning off virtualization could be useful here, at that point the table does little more than render rows for you - something that you could easily do on our own (and you'd have a lot more control over state, content, etc). If you aren't rendering large quantities of data and you stateful cells are a requirement, it may be of better use to you to render your own table, and possibly paginate if you do run into performance setbacks.

As for performance - we find a performance benefit in our use case where we deal with hundreds if not thousands of rows of data (FDT has been tested with ~1000000 rows!) and want to maintain consistent performance no matter how many rows we may need to render or change.

I'll keep this open though, since it is something we should address - but it might not make it into the next release.

sidnair commented 9 years ago

I'll be rendering up to a few thousand rows, so virtualization would help, but having stateful cells is also important.

I think I'll start by forking the repo and turning off virtualization (since I understand why you wouldn't want that as part of the API) and see how performance is. If performance is an issue, I'll make the cells stateless or paginate. And when you add support for stateful cells, I can upgrade to the new version more easily than if I rolled my own table.

Thanks again for the help.