Skip to content
This repository has been archived by the owner on Jan 27, 2021. It is now read-only.

Compatibility issue with react-redux v6 #121

Closed
mickorand opened this issue Sep 2, 2019 · 3 comments
Closed

Compatibility issue with react-redux v6 #121

mickorand opened this issue Sep 2, 2019 · 3 comments
Labels

Comments

@mickorand
Copy link

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
Copy link
Contributor

mpeyper commented Sep 3, 2019

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.

@mpeyper mpeyper added the bug label Sep 3, 2019
@mpeyper mpeyper changed the title State mapping Compatibility issue with react-redux v6 Sep 3, 2019
@mickorand
Copy link
Author

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

@mpeyper
Copy link
Contributor

mpeyper commented Sep 8, 2019

Updated peerDependency was released in v5.0.0.

@mpeyper mpeyper closed this as completed Sep 8, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

No branches or pull requests

2 participants