ioof-holdings / redux-subspace

Build decoupled, componentized Redux apps with a single global store
https://ioof-holdings.github.io/redux-subspace/
BSD 3-Clause "New" or "Revised" License
312 stars 33 forks source link

Compatibility issue with react-redux v6 #121

Closed mickorand closed 5 years ago

mickorand commented 5 years ago

Is it a bug, feature request or question?

Bug

Which package(s) does this involve?

react-redux / react-redux-subspace

Input Code

I'm not sure if the actual code matters, but here's an idea of setup (it's pretty basic on it's own):

State:

{
    filterOneResults: {
        criteria: string;
        results: string[];
    }
}

Parent (multiple filtered lists in a scroll view):

<SubspaceProvider mapState={state => state.filterOneResults} namespace="filterOneResults">
    <FilteredList filter={filter} />
</SubspaceProvider>

Child (given the provided filter dispatches the request and waits for data back):

class FilteredView extends React.PureComponent<*> {
    ...
}

const mapStateToProps = state => ({
    results: state.results,
});

const mapDispatchToProps = dispatch => ({
    dispatch: dispatch,
});

export default connect(mapStateToProps, mapDispatchToProps)(FilteredList);

Expected Behavior

Expecting to receive next type of object in mapToState function of FilteredList component:

{
    criteria: string;
    results: string[];
}

Current Behavior

Receiving next type of object in mapToState function of FilteredList component:

{
    filterOneResults: {
        criteria: string;
        results: string[];
    }
}

Possible Solution

Checked the setup multiple times and switched to debugging. It appears to me that the issue dwells in between react-redux-subspace and react-redux.

Namely, SubspaceProvider from react-redux-subspace updates the store for its children here:

const useReplacedContext = (context, store) => {
  const contextValue = useContext(context)
  return useMemo(
    () => ({ ...contextValue, store }),
    [contextValue, store]
  )
}

Please mention that we only update the store in the contextValue, but not the storeState. Whereas in the react-redux we use the storeState. Namely, in the line var nextProps = sourceSelector(state, props):

function makeDerivedPropsSelector() {
  var lastProps;
  var lastState;
  var lastDerivedProps;
  var lastStore;
  var lastSelectorFactoryOptions;
  var sourceSelector;
  return function selectDerivedProps(state, props, store, selectorFactoryOptions) {
    if (pure && lastProps === props && lastState === state) {
      return lastDerivedProps;
    }

    if (store !== lastStore || lastSelectorFactoryOptions !== selectorFactoryOptions) {
      lastStore = store;
      lastSelectorFactoryOptions = selectorFactoryOptions;
      sourceSelector = selectorFactory(store.dispatch, selectorFactoryOptions);
    }

    lastProps = props;
    lastState = state;
    var nextProps = sourceSelector(state, props);
    lastDerivedProps = nextProps;
    return lastDerivedProps;
  };
}

It looks like adding storeState: store.getState() in useReplacedContext function fixes the problem:

const useReplacedContext = (context, store) => {
  const contextValue = useContext(context)
  return useMemo(
    () => ({ ...contextValue, store, storeState: store.getState() }),
    [contextValue, store]
  )
}

Your Setup

package version(s)
redux 4.0.1
react-redux 6.0.1
redux-subspace 4.0.0
react-redux-subspace 4.0.0
redux-saga 1.0.2
redux-subspace-saga 4.0.0

Context

It might be that I'm doing something wrong, but I rechecked the examples and my code somewhat 10 times now and ask a colleague to do so as well to be sure. It seems that everything is according to usage examples, yet the situation described above happens. In case my understanding of how it's supposed to work is wrong (e.g. storeState should carry on the global state) please advise how it supposed to be.

mpeyper commented 5 years ago

Ah, this is potentially a difference between react-redux v6 and v7 that I did not factor in when specifying the allowed versions. In the current version of react-redux, there is no storeState in the context. Is it possible for you to update to v7.1.1 and seeing if that works for you?

I don't think there is be any huge issue in providing the additional value to the context to support v6 still, but we will soon be making a change to use a new Provider instead of messing around with the context directly. There was a bug in Provider that prevented us from doing this initially, which has now been fixed, so we are going to make that switch soon and drop v6 support all together, so it may be in your best interest to update anyway, even if just for the performance improvements.

mickorand commented 5 years ago

Yup that helped. Thanks for the quick response. Made a PR to update the peerDependencies.

mpeyper commented 5 years ago

Updated peerDependency was released in v5.0.0.