Fix current location being set unnecessaily with server side rendering

[khankuan]

Fixes #410

It seems like the currentLocation is always set when location change. However, this is unnecessary on server side rendering when location is already present in the store. Although both location are logically the same, they are actually different objects.

@dlmr Do double check if this PR works for your project.

On side note, I think this bug might be present even before 4.0.5 but might not have encountered for other reasons.

[dlmr]

I now understand what is happening and you are correct in that there is a problem that needs to be addressed.

This fix works in my project but I'm however not certain that this is the correct way of doing it since as you mentioned the key in history.state will be out of sync with what we will have in the reducer. Currently this will be "fixed" when history is changed and we do have initialLocation defined to something. I'm at this time also not sure how this change will affect users that are not using server side rendering/getting an initial state from the server.

I will look more into this and get back to this PR when I have a better grasp of what is happening and the possible edge cases that might exist.

@timdorr Do you know if it is important that the key is the same in the reducer and the history state? If that is the case it might be a good idea to dispatch an action on initialisation on the client but NOT dispatch a history action, the reason why #313 occurred. I have a POC of this that seems to work, I will look more into this going forward.

[timdorr]

@dlmr Having the same key is very important, as it's the primary means of identifying an "instance" of a location descriptor. E.g., you navigate throughout an app and land on the same path twice, you may need to distinguish between which location descriptor is which.

It looks like we need more extensive testing around initialization behaviors. I've been bogged down at work, so I'm really only able to keep my head above water with issues at the moment. I'll see about beefing up our test suite the next time I can take a pass at things.

If anyone wants to add a PR with tests (including breaking ones!), that would be awesome.

[hung-phan]

I also ran into the same problem without the server rendering when using redux-immutable.

reducer.js

export default handleActions({
  [LOCATION_CHANGE]: (state, { payload }) => state.set('locationBeforeTransitions', payload),
}, fromJS({ locationBeforeTransitions: null }));

routes.js

const selectLocationState = (state) => state.get('routing').toJS();

export const getClientHistory = (store) =>
  syncHistoryWithStore(
    require('react-router').browserHistory,
    store, { selectLocationState }
  );

todo.js

// @flow
import React from 'react';
import { List } from 'immutable';
import { compose, shouldUpdate } from 'recompose';
import shallowEqualImmutable from 'react-immutable-render-mixin/lib/shallowEqualImmutable';
import { bindActionCreators } from 'redux';
import { connect } from 'react-redux';
import TodosHeader from './todos-header';
import TodosAdd from './todos-add';
import TodosBody from './todos-body';
import TodosFooter from './todos-footer';
import fetchDataEnhancer from 'client/helpers/fetch-data-enhancer';
import { addTodo, removeTodo, completeTodo, fetchTodos } from './logic-bundle';
import type { Todo } from './types';

export const Todos = ({ todos, actions }: { todos: List<Todo>, actions: Object }) => (
  <div className="container">
    <div className="row">
      <TodosHeader />
      <TodosAdd addTodo={actions.addTodo} />
      <TodosBody
        todos={todos}
        removeTodo={actions.removeTodo}
        completeTodo={actions.completeTodo}
      />
      <TodosFooter />
    </div>
  </div>
);

Todos.propTypes = {
  todos: React.PropTypes.instanceOf(List).isRequired,
  actions: React.PropTypes.object,
};

export const enhance = compose(
  fetchDataEnhancer(
    ({ store }) => store.dispatch(fetchTodos())
  ),
  connect(
    state => ({
      todos: state.get('todos'),
    }),
    dispatch => ({
      actions: bindActionCreators({
        addTodo,
        removeTodo,
        completeTodo,
      }, dispatch),
    })
  ),
  shouldUpdate((props, nextProps) => !shallowEqualImmutable(props.todos, nextProps.todos))
);

export default enhance(Todos);

Function fetchDataEnhancer will be triggered via route changing using redial.

  history.listen(location => {
    match({ routes, location }, (error, redirectLocation, renderProps) => {
      if (error) {
        navigateTo('/500.html');
      } else if (redirectLocation) {
        navigateTo(redirectLocation.pathname + redirectLocation.search);
      } else if (renderProps) {
        if (!isEmpty(window.__data)) {
          // Delete initial data so that subsequent data fetches can occur
          delete window.__data;
        } else {
          // Fetch mandatory data dependencies for 2nd route change onwards
          trigger('fetchData', renderProps.components, getLocals(store, renderProps));
        }
      } else {
        navigateTo('/404.html');
      }
    });
  });

Just my guess, the location are the same but they are different objects. state.get('routing').toJS() will definitely create new object every time. The above solution works in my case. My current workaround is to create a nested level for the immutable and store it in plain object. I would love to see any suggestion on this problem

[hung-phan]

I rechecked this again. This only happens with server rendering.

[slowsay]

should be disposed combineReducers({function,function})

[timdorr]

Going with #445 instead. Thanks for the PR nonetheless!