bencripps / react-redux-grid

A React Grid/Tree Component written in the Redux Pattern
http://react-redux-grid.herokuapp.com/
MIT License
445 stars 63 forks source link

Does not refresh after SET_DATA action #19

Closed kkwak closed 8 years ago

kkwak commented 8 years ago

Issue: grid doesn't show the data

  1. Plugged in my own store - added all the reducers from Reducers
  2. I've got the grid all plugged in ( shows "No data available") by passing a promise to the dataSource prop.
  3. I can see that the SET_COLUMNS action is raised properly
  4. I can verify that the dataSource is called and processed - and returns a valid value, subsequently SET_DATA is raised with the "data" prop populated.
  5. on the component containing the , I can see that the componentWillReceiveProps() is called - and that the props have a "dataSource" entry which is a map with the "lastUpdate": 2 and "myGridKey". a. "myGridKey" entry is also a Map { "data": List [ Map { {entry1}, {entry2}, etc } ] }

The grid shows nothing at this point. My app hooks in to hot loading and when a file is changed and the hot loader is invoked, the grid shows the data (So I know somewhere everything is configured properly!).

I am completely lost at this point - I don't think I've missed anything, but let me know if someone could assist.

Thank you!

bencripps commented 8 years ago

@kkwak thanks for posting the issue; I know what the immediate problem is, and it's not your configuration, as you inferred.

The quick fix is this:

Rather than doing as the documentation suggests:

import { Reducers } from 'react-redux-grid';

const myReducers = {
    myCustomReducer,
    ....Reducers
};

Do this:

import { Reducers } from 'react-redux-grid';

const gridReducers = {
    dataSource: Reducers.DataSource,
    editor: Reducers.Editor,
    errorHandler: Reducers.ErrorHandler,
    filter: Reducers.Filter,
    grid: Reducers.grid,
    loader: Reducers.Loader,
    menu: Reducers.Menu,
    pager: Reducers.Pager,
    selection: Reducers.Selection
};

export const rootReducer = combineReducers({
    app,
    ...gridReducers
});

This will fix your issue.

The coming fix for grid

I haven't decided how I'm going to fix this. Currently there are two components that read state out of the store -- the stateGetter utility, and the shouldComponentUpdate function for the central grid component. The stateGetter function is case insensitive, and thus didn't break, however, the shouldComponentUpdate function isn't case insensitive (yet).

Proposal 1

Make shouldComponentUpdate case insensitive. This is the simpler fix, and it makes the grid more fault protective, but it also slows down performance because it has to normalize values on every function call -- this is mitigated somewhat, by only checking for case sensitivity if the values don't match on first run.

Proposal 2

Update the core exports to correct casing on the reducers => Reducers.DataSource would be Reducers.dataSource. This is the cleaner fix, but it would break current users of the grid.

The Reducer list would look as follows:

const Reducers = {
    dataSource: datasource,
    grid: grid,
    bulkActions: bulkaction,
    editor: editor,
    errorHandler: errorhandler,
    filter: filter,
    loader: loader,
    menu: menu,
    pager: pager,
    selection: selection
};

I'm going to think about this for a couple days before I commit the changes. I also may allow for both for a short time, emitting console warns until the next major release.

Let me know if the fix above doesn't work, but I'm almost certain that's the problem.

kkwak commented 8 years ago

You are absolutely correct. Didn't realize that this was due to casing! I was looking all over the place in the code..

I like Proposal 2 with the warnings.. if it makes a difference. Additionally, would it be possible to scope the store names? dataSource, grid, etc are very common names. Would be nice if it was prefixed by rrg-dataSource, rrg-grid, rrg-bulkActions, etc.. As the default for any app would be to provide an existing store.

Thanks for the fast feedback. Hoping this grid will work for me!

bencripps commented 8 years ago

Awesome! Glad that worked for you. I think I'm going to go with proposal 2, allowing for case insensitivity for the short duration.

Also, I love the idea of namespacing the reducers. However, this will also be a breaking change, so it will need to be made as a separate MR with full documentation of the changes. I will do that after I make the changes for the casing issue.

Thanks!