GriddleGriddle / Griddle

Simple Grid Component written in React
http://griddlegriddle.github.io/Griddle/
MIT License
2.5k stars 378 forks source link

re-rendering does not occur because of the object's comparison with itself. #769

Open radziksh opened 7 years ago

radziksh commented 7 years ago

Griddle version

1.10.1

Expected Behavior

hard to say, perhaps a more optimal way to update the state of the grid if it is controlled from the outside.

Actual Behavior

not so optimal. Problem with pageProperties

Steps to reproduce

My grid is controlled from the outside and all props are passed by reference so that the grid update function (src/index.js)

componentWillReceiveProps(nextProps) {
    const newState = _.pickBy(nextProps, (value, key) => {
      return this.props[key] !== value;
    })

    // Only update the state if something has changed.
    if (Object.keys(newState).length > 0) {
     this.store.dispatch(actions.updateState(newState));
    }
  }

works correctly. Here is the grid code:

export class UsersRequestsGrid extends React.Component<TUsersRequestsGridProps, {}> {
  constructor(props) {
    super(props);
    this.events = {
      onSort: (params) => {

        const columnId = params.id;
        this.props.gridActionCreators.setSortedColumn(_.upperFirst(columnId));

        if (this.props.gridState.sortOrder === "desc") {
          this.props.gridActionCreators.setSortOrder("asc");
        } else {
          this.props.gridActionCreators.setSortOrder("desc");
        }

        this.props.gridActionCreators.updateGridFromServer();
      },
      onNext: () => {
        this.props.gridActionCreators.setCurrentPage(this.props.gridState.currentPage + 1);
        this.props.gridActionCreators.updateGridFromServer();
      },

      onPrevious: () => {
        this.props.gridActionCreators.setCurrentPage(this.props.gridState.currentPage - 1);
        this.props.gridActionCreators.updateGridFromServer();
      },

      onGetPage: (currentPage) => {
        this.props.gridActionCreators.setCurrentPage(currentPage);
        this.props.gridActionCreators.updateGridFromServer();
      }
    };

    this.events.onSort = this.events.onSort.bind(this);
    this.events.onNext = this.events.onNext.bind(this);
    this.events.onPrevious = this.events.onPrevious.bind(this);
    this.events.onGetPage = this.events.onGetPage.bind(this);

    this.pageProperties = {
      pageSize: props.gridState.itemsPerPageCount,
      currentPage: props.gridState.currentPage,
      recordCount: props.gridState.recordCount,
    };
  }
  events: any;
  pageProperties: any;
  // workaround:
  componentWillReceiveProps(nextProps: TUsersRequestsGridProps) {
    if (nextProps.gridState.itemsPerPageCount !== this.props.gridState.itemsPerPageCount
      || nextProps.gridState.currentPage !== this.props.gridState.currentPage
      || nextProps.gridState.recordCount !== this.props.gridState.recordCount) {
      this.pageProperties = {
        pageSize: nextProps.gridState.itemsPerPageCount,
        currentPage: nextProps.gridState.currentPage,
        recordCount: nextProps.gridState.recordCount,
      };
    }
  }

  render() {
    return (
      <Griddle
        components={components}
        storeKey="griddleStore"
        data={this.props.gridState.requests}
        events={this.events}
        pageProperties={this.pageProperties}
        styleConfig={styleConfig}
      >
        {
          this.props.gridState.requestType === "Outgoing"
            ? outgoingRows
            : incomingRows
        }
      </Griddle>
    );
  }
}

The problem is that in the componentWillReceiveProps in the line:

var newState = (0, _pickBy3.default) (nextProps, function (value, key) {
     return _this2.props [key]! == value; // pageProperties === pageProperties is true because it is the same object
});

pageProperties are compared with themselves and the state of the grid is not updated. Therefore, I have to manually kill the link to the old pageProperties with this ugly code:

 componentWillReceiveProps(nextProps: TUsersRequestsGridProps) {
    if (nextProps.gridState.itemsPerPageCount !== this.props.gridState.itemsPerPageCount
      || nextProps.gridState.currentPage !== this.props.gridState.currentPage
      || nextProps.gridState.recordCount !== this.props.gridState.recordCount) {
      this.pageProperties = {
        pageSize: nextProps.gridState.itemsPerPageCount,
        currentPage: nextProps.gridState.currentPage,
        recordCount: nextProps.gridState.recordCount,
      };
    }
  }

I noticed that if pageProperties were passed directly to the griddle, for instance:

<Griddle
      pageSize: this.props.itemsPerPageCount
      currentPage: this.props.gridState.currentPage,
      recordCount: this.props.gridState.recordCount
/>

then this problem would not arise (since simple types are passed by value and the comparison function would work correctly) and the grid would continue to be updated at the right time. So my question is there some other way to not write this awful construction in the componentWillReceiveProps?

dahlbyk commented 7 years ago

My grid is controlled from the outside and all props are passed by reference

The unanimous React/Redux best practice would be to avoid mutation of your gridState object. Can you share any more about how your gridState is actually being updated?

radziksh commented 7 years ago

@dahlbyk because I have several requestsGrid in different places I reuse reducer via createFilteredReducer

export const gridNameAdminDashboard = "ADMIN_REQUESTS_GRID";
export const pageReducerAdminDashboard = combineReducers({
  requestsGrid: createFilteredReducer(requestsGridReducer, act => o(act.meta).name === gridNameAdminDashboard)
});

here is factory:

export function createFilteredReducer(reducerFunction, reducerPredicate) {
    return (state, action) => {
        const isInitializationCall = state === undefined;
        const shouldRunWrappedReducer = reducerPredicate(action) || isInitializationCall;
        return shouldRunWrappedReducer ? reducerFunction(state, action) : state;
    };
}

here is reused reducer:

export const requestsGridReducer: Reducer<TRequestsGridState> = (state = initialState, action: TAction<any>) => {
  switch (action.type) {
    case requestsGridActionNames.SET_ITEMS_PER_PAGE_COUNT: {
      return updateState(state, {
        itemsPerPageCount: action.payload,
      });
    }
    case requestsGridActionNames.SET_CURRENT_PAGE: {
      return updateState(state, {
        currentPage: action.payload
      });
    }
    ...
    default:
      return state;
  }
};

here is immutable updateState function

import * as Immutable from "seamless-immutable";
export function updateState<TState>(state: TState, forMerge: Partial<TState>) {
  return Immutable.merge(state, forMerge, { deep: true });
}
dahlbyk commented 7 years ago

@radziksh I'm not sure I see in this where you end up reusing a modified grid state? It seems like createFilteredReducer produces a reducer that only gets next state from requestsGridReducer, which always returns a new Immutable.

radziksh commented 7 years ago

My grid is controlled from the outside and all props are passed by reference

maybe I said a little wrong, of course I follow this agreement on immutability, I mean that if I write:

<Griddle
   pageProperties={{
      pageSize: this.props.pageSize
      ...
   }}
/>

then if an event occurs that is not directly related to the grid, the function for updating the state of the grid will begin its work:

]componentWillReceiveProps(nextProps) {
    const newState = _.pickBy(nextProps, (value, key) => {
      return this.props[key] !== value;
    })

    // Only update the state if something has changed.
    if (Object.keys(newState).length > 0) {
     this.store.dispatch(actions.updateState(newState));
    }
  }

Since each time from its point of view a new object is transferred into it. And this is undesirable behavior, because in reality nothing has changed. And in our case these events occur constantly when the user moves the mouse (The application monitors whether the user is active to show the message about the automatic exit from the application in case of his inactivity) + 1 time per second due to custom timers :(

but if I create a reference to pageProperties in the parent object, then the opposite problem is obtained. Grid is not updated when he should. So I have to use workaround in componentWillReceiveProps - example in first message.

<Griddle
   pageProperties={this.pageProperties}
/>