connect assumes store prop is a redux store

[aaronjensen]

Do you want to request a feature or report a bug?

(If this is a usage question, please do not post it here—post it on Stack Overflow instead. If this is not a “feature” or a “bug”, or the phrase “How do I...?” applies, then it's probably a usage question.)

What is the current behavior?

When you pass a store prop to a connected component, it attempts to use the value of the prop as a redux store. This is wreaking havoc on our codebase that has store as a commonly passed prop. I haven't been able to find any documentation for this behavior and I can't find a way to override it, though it does seem to be intentional:

https://github.com/reduxjs/react-redux/blob/master/src/components/connectAdvanced.js#L197

It even seems at one point there was a way to override this behavior:

https://github.com/reduxjs/react-redux/blob/master/src/components/connectAdvanced.js#L115

I'm filing this as a bug because as far as I can tell, #1000 removed this behavior and disabled the override key and then #1209 added it back in, without reenabling the override key. My guess is that this wasn't intentional and hope that adding the override key back would be acceptable.

If the current behavior is a bug, please provide the steps to reproduce and if possible a minimal demo of the problem. Your bug will get fixed much faster if we can run your code and it doesn't have dependencies other than React. Paste the link to a CodeSandbox (https://codesandbox.io/s/new) or RN Snack (https://snack.expo.io/) example below:

Repro is obvious, but I can add one if desired.

What is the expected behavior?

Either connect would not use the store prop at all, or the name would be overridable (likely preferred since it would be a less breaking change).

Which versions of React, ReactDOM/React Native, Redux, and React Redux are you using? Which browser and OS are affected by this issue? Did this work in previous versions of React Redux?

react-redux 7.1.1
redux 4.0.4
react/react-dom 16.8.3

[markerikson]

This behavior has existed in every version of React-Redux except v6. The only reason it didn't exist in v6 was because connected components didn't subscribe to the store directly in that version. All other versions have accepted a prop named store, and subscribed directly to that store if provided as a prop.

I'm curious how you didn't run into this previously. Has your codebase only used React-Redux v6 so far, and you just now upgraded to v7?

I agree it should probably be a bit better documented, but this is well known behavior, and not something we're planning to change at this time. I also don't see a need to specifically re-enable the "override key" behavior either.

I'm going to close this for now. If you have very strong reasons why this should be reconsidered, though, please feel free to keep discussing this further, and we can talk through the issue.

[aaronjensen]

Hi @markerikson thanks for getting back to me so quickly. The codebase is one we are migrating to Redux from an alternate flux implementation, so v7 is the first version it has used. It has dozens (maybe 100?) connected components that take a store prop (which means a physical store, not a redux store). I've started to rename that prop to shop, but it's a game of whack-a-mole that I'd rather not play, plus it introduces a new term that's only used in some places on the client and not others, and none in the server. Ultimately, it adds confusion.

I've considered writing a wrapping HoC to do the prop renaming, but in order to support the full interface of connect and transparently rename the store prop to something else and then rename it back when calling mapStateToProps (for example), would take a decent amount of work.

Restoring this behavior in redux seemed to be the path of least resistance. I totally understand why it exists, and it's a convenient thing for testing and other reasons I'm sure, but coopting the store prop has a pretty heavy impact on this migration for us.

For posterity, it appears storeKey was only introduced in 5.0.0: ee4366b around 3 years ago and then removed and made unnecessary in v6, as you said.

[markerikson]

Yeah, to clarify: the notion of <ConnectedComponent store={someReduxStore} /> has existed in every version except v6. v5 did introduce the notion of storeKey, which went away in v6 when we switched to having only <Provider> subscribe. When I reimplemented per-component subscriptions in v7, I didn't bother with storeKey, as it didn't seem like there was any real benefit.

Interestingly, I think the original intent of storeKey was almost more for what context field should be read, although the code did use it to check both props[storeKey] and context[storeKey].

Digging through the issues, the most relevant historical discussions are:

#693
#695
#838
#942
#1132

I understand the situation you're in, and I sympathize. Migrations are always tough, and more so when you're having to fight the tools you're dealing with. At the same time, I'm very hesitant to (re-)add something to our public API that is only for a very niche use case, and that no one else has expressed an interest in so far.

As an alternate idea: we could potentially tweak the logic in connect so that it only treats props.store as the Redux store if it actually looks and quacks like a Redux store. Notionally:

      // The store _must_ exist as either a prop or in context
-     const didStoreComeFromProps = Boolean(props.store)
+     const didStoreComeFromProps = Boolean(props.store) && props.store.getState && props.store.dispatch      
      const didStoreComeFromContext =
        Boolean(contextValue) && Boolean(contextValue.store)

      invariant(
        didStoreComeFromProps || didStoreComeFromContext,
        `Could not find "store" in the context of ` +
          `"${displayName}". Either wrap the root component in a <Provider>, ` +
          `or pass a custom React context provider to <Provider> and the corresponding ` +
          `React context consumer to ${displayName} in connect options.`
      )

-     const store = props.store || contextValue.store
+     const store = didStoreComeFromProps ? props.store : contextValue.store

connect appears to actually already pass through props named store, it's just that it's trying to subscribe as soon as it sees one. If we convince it to not subscribe, I think that might be sufficient.

@timdorr , thoughts?

[aaronjensen]

I totally understand not wanting to add to the public API for just us. Your proposed solution sounds great to me—I think it's a good idea.

[aaronjensen]

I think I've worked around it successfully. Can you see any problems with this approach, @markerikson, or is there a better way to accomplish it?

It'd still be great if your suggestion was implemented so I could remove this wrapper.

const fixProps = ({ __store, ...props }) => ({ ...props, store: __store })

export const connect = (
  mapStateToProps,
  mapDispatchToProps,
  mergeProps,
  options
) => WrappedComponent => {
  const ConnectComponent = reduxConnect(
    mapStateToProps && mapStateToProps.length > 1
      ? (state, ownProps) =>
          mapStateToProps(state, fixProps(ownProps))
      : mapStateToProps,
    mapDispatchToProps && mapDispatchToProps.length > 1
      ? (dispatch, ownProps) =>
          mapDispatchToProps(dispatch, fixProps(ownProps))
      : mapDispatchToProps,
    (stateProps, dispatchProps, ownProps) => {
      const fixedOwnProps = fixProps(ownProps)

      return mergeProps
        ? mergeProps(stateProps, dispatchProps, fixedOwnProps)
        : { ...fixedOwnProps, ...stateProps, ...dispatchProps }
    },
    options
  )(WrappedComponent)

  const ProtectedComponent = ({ store, ...props }) => {
    return React.createElement(ConnectComponent, { ...props, __store: store })
  }

  ProtectedComponent.WrappedComponent = WrappedComponent

  return hoistStatics(ProtectedComponent, ConnectComponent)
}

[markerikson]

Mmm... you could probably try customizing the mergeProps argument to connect or something like that?

https://react-redux.js.org/api/connect#mergeprops-stateprops-dispatchprops-ownprops-object

[aaronjensen]

That only handles the final props, I want it to be transparent in any usage of ownProps, including mapDispatchToProps and mapStateToProps. Also, I think if I just did that, connect would still get store and blow up

[markerikson]

Belatedly fixed in https://github.com/reduxjs/react-redux/releases/tag/v7.1.2 . Hope this helps!

[aaronjensen]

It helps greatly, thank you! We just completed our redux migration, so now I can remove my workaround.

[markerikson]

Great! Next suggestion: go try out https://redux-starter-kit.js.org :)

(maybe I shoulda said that before you got involved in the migration...)

[aaronjensen]

Ah, yeah that may have helped. Looks great. I might suggest a docs page about typescript, looks like you have decent support for it, but I had to dig into the code to find it. Also, not sure why this is tested for: https://github.com/reduxjs/redux-starter-kit/blob/master/type-tests/files/createSlice.typetest.ts#L84

I'd prefer the type was the actual literal, but that's just me. Cheers!

[markerikson]

We can't actually make the types true literals, because we're doing concatenation: ${prefix}/{reducerName}. Even when those are known at runtime, that can't become a literal.

Yeah, we don't have a real docs page on usage with TS, but the Advanced Tutorial page effectively covers that.