reactdatagrid icon indicating copy to clipboard operation
reactdatagrid copied to clipboard

Switching columns and dataSource props, the new columns are called with old data

Open nstoertz opened this issue 4 years ago • 9 comments

@inovua/reactdatagrid-enterprise: 4.1.15

When I switch columns and dataSource props, the new columns are called once with the old data, when I'd expect to always get the new data.

Here's a repro case: https://codesandbox.io/s/serene-bird-spc5d

When fixing this, please make sure it also works when switching from a TreeGrid to normal grid rendering.

nstoertz avatar May 07 '21 20:05 nstoertz

Yes, I remember seeing this one too. The reason we could get away with it is we always access the data object in a safe manner dereferencing one level at a time. Users do not see the 'empty' cells of the first render because the second render happens very fast and 'overrides' the first one. Would be nice to see it fixed though as extra rendering slows things down for sure.

yurigenin avatar May 07 '21 21:05 yurigenin

Our problem is not with performance - it's that it's very cumbersome for us to program all columns to be compatible with all data sources.

For example, if I have a TreeGrid specific columns that I expect to use be passed nodeProps, I now need to put code like this in my column render method:

        render: (...args: any[]) => {
            ...
            // When switching data sources and calumns, the grid temporarily
            // calls the new columns with data from the old data source
            // TODO(nassar): Remove once Inovua fixes https://github.com/inovua/reactdatagrid/issues/133
            if (!nodeProps) {
              return '';
            }

This is OK to do when there are only a few combinations of columns / datasources, but more difficult when there are hundreds.

The repro case I attached shows a simpler example of when this could cause a bug. Let me know if you have trouble accessing it for some reason.

nstoertz avatar May 10 '21 16:05 nstoertz

We're trying to find a solution to this - though it's a tricky one as it needs props coordination - your new datasource could be a promise that resolves in 10 seconds, while the columns should re-render immediately. A solution could be to reset the datasource to an empty array as soon as we detect a change of datasource, thus the render between columns and datasource should be coordinated. We're trying to implement this fix.

inovua-admin avatar May 17 '21 14:05 inovua-admin

Glad you are working on a solution! Resetting the grid to have no data between the time the dataSource reference changes and when the new dataSource resolves seems like a good solution to me.

Longer term, you might consider an alternative interface to notify the grid of data changes without needing to change the datasource object reference. Lots of prior art here, eg:

  • https://www.ag-grid.com/javascript-grid/view-refresh/
  • https://developer.android.com/reference/android/widget/BaseAdapter#notifyDataSetChanged()
  • https://developer.apple.com/documentation/uikit/uicollectionview/1618078-reloaddata
  • https://developer.android.com/reference/androidx/recyclerview/widget/AsyncListDiffer
  • https://developer.apple.com/documentation/uikit/uicollectionview/1618045-performbatchupdates

nstoertz avatar May 25 '21 19:05 nstoertz

Any update here?

nstoertz avatar Jun 28 '21 18:06 nstoertz

Any update here? Our current workaround is to unmount the grid and remount after a timeout, but this causes the grid to flash and is not ideal. We could potentially render a separate empty grid temporarily while we wait for the new data to load, but I'd love to know if you all are still working on this before investing more time.

nstoertz avatar Jul 26 '21 16:07 nstoertz

If you don't mind my asking, how exactly are you unmounting/remounting? Are you doing something like conditionally rendering the whole RDG based on some kind of "switch is in progress" state?

We have a similar issue — a big grid system that can display different datasources, each of which has its own columns, relevant filters and sorts, etc (although some datasources share some columns). So when the user changes datasources, we sometimes (but, maddeningly, not always!) get a sprinkling of columns that are missing filter UI (if the new and previous datasources had different filter options), or sometimes even a "no records available" grid (if a prior filter is somehow being applied to the new datasource that doesn't have the same data). The "blank grid" issue resolves by manually doing a "clear all" on any filter — which is, in effect, what we need RDG to be able to do programmatically: recognize the change and set up a "fresh" environment.

So it seems we have a family of issues that all revolve around needing the grid system to generally reinitialize itself or, more specifically, fire lifecycle triggers in some way when certain events occur.

redler avatar Oct 05 '21 20:10 redler

I was wrapping the grid component and briefly rendering empty content when dataSource, columns, or other props change.

There were several downsides... callers need to be careful to pass memoized prop values, callers need to handle the gridRef being temporarily unset, and we lose some state we'd like to keep, like expanded rows. We ended up doing a combination of making our columns able to be called with bad data, and calling private methods on the grid in order to attempt to reset data, eg gridRef?.current?.setSkip?.(0); gridRef?.current?.silentSetData([]);

In the end, I'd like to see this fixed not by automatically having the grid reinitialize but by:

  1. Fixing this ticket, where our the most recent columns passed to the grid can be called with data that isn't from the most recent data source. Instead, the old columns should be used while the data source refetches.
  2. Add an explicit reloadData method that we can use to refetch data ourselves. Ticket here: https://github.com/inovua/reactdatagrid/issues/122

It might be reasonable to internally refetch any time the dataSource changes as well, but you might want to make that an opt-in property, like shouldReloadOnDataSourceChange. While reloading, I also think it reasonable to render a loading state, along with the most recent set of columns.

nstoertz avatar Oct 05 '21 20:10 nstoertz

Similar here. Our workaround was to conditionally swap out the Grid for our regular "loading" overlay based on what amounts to an isSwitching flag in state, then just blipping that flag as part of the other switching machinery (new data fetch, etc) that kicks in when a user switches data sources. It works, but it's not great. I'd upvote your suggestions more than once if I could. We do of course pay for Enterprise, but it's cheap, and frankly I'd pay much more if doing so meant helping the Inovua team look into these issues and expand their engagement here.

redler avatar Oct 05 '21 21:10 redler