Pass through props.context to connected component

[cmfcmf]

This issue came up when I tried to update connected-react-router to work with react-redux@7 (see supasate/connected-react-router#303).

In react-redux@6, you were able to do the following:

const myContext = React.createContext(null);

function MyComponent(props) {
  assert(props.context === myContext); // true
}
const ConnectedMyComponent = connect()(MyComponent);

// ...

<ConnectedMyComponent context={myContext} />

With react-redux@7, the context prop is no longer passed through to MyComponent. This PR "fixes" that. However, I'm really not an expert when it comes to React's context API and its usage in react-redux and connected-react-router. Let me know if this fix makes sense :)

[netlify]

Deploy preview for react-redux-docs ready!

Built with commit 70e0b08

https://deploy-preview-1278--react-redux-docs.netlify.com

[markerikson]

Hmm. If we really were passing through props.context to the wrapped component in v6, I'd consider that to actually be a bug. Much like key and ref are instructions to React and are pulled off before your component ever sees them, I would view store and context as being the same way for React-Redux.

[timdorr]

I don't see any harm in make them available props. Since you can't do this.context anymore, this serves as a replacement.

[timdorr]

Without any major objections, I'm going to merge this in. If Mark feels strongly about it, we can certainly revert.

[markerikson]

I don't feel too strongly about it, but it does sorta feel like an implementation detail leaking into the wrapped component.

The better question is, why do you feel a need to access the React-Redux context instance, in the wrapped component, as a prop?

[cmfcmf]

The better question is, why do you feel a need to access the React-Redux context instance, in the wrapped component, as a prop?

Here's the relevant code from connected-react-router:

https://github.com/supasate/connected-react-router/blob/37e90d7a55717046d4265f5832d3f9c8356e7db8/src/ConnectedRouter.js#L105-L123

I don't know why its implemented like that right now – but it would probably be possible to implement it differently.

[jeffberry]

I'm a little concerned about this change, we're unable to upgrade to react-redux@7 due to the context prop being forced into components now. The problem is, that we used context as a prop name in a few scenarios.

My concern is not based on our blocking issue, because admittedly context was far too generic of a prop name for us to choose anyway, but I fear this is a breaking change and largely unnecessary. We are now passing context in as a prop to be evaluated by lifecycle methods for equality. Context should be accessed via context, not as a prop. Why was this change required?

The link that @cmfcmf posted shows ConnectedRouter trying to pull context from a prop, but I believe that is only allowing the ability to use your own context instead of the included ReactReduxContext, like so:

const myContext = React.createContext();

render(
  <ConnectedRouter context={myContext} />
);

[markerikson]

The more I think about this, the less I like it. @timdorr , I think we ought to revert this change.

That said, @jeffberry , I don't think this actually made it out into a release.

[timdorr]

@jeffberry This doesn't change anything on the incoming props side of thing. It only changes the outgoing props to the wrapped component, which will now include the context prop. Prior to this, it was being stripped, regardless of what you were using it for.

// 7.0.0
<ConnectedThing context={context} {...otherProps} /> => <Thing {...otherProps} />
// This PR:
<ConnectedThing context={context} {...otherProps} /> => <Thing context={context} {...otherProps} />

So, even though it still needs to be a React Context, it's no longer hidden from the wrapped component.

[markerikson]

So there's two different issues here:

• context is now a "reserved word" prop for React-Redux, in addition to store. Similarly to how React itself reserves key and ref, you can no longer meaningfully have props with those names being passed to connected components - <MyConnectedComponent context={someAppData} /> will break.
  • At the same time, similar to how React strips off key and ref so your component never sees them, up until now we have not passed context and store on to the wrapped component
• For this PR specifically, we now pass context on to the wrapped component. I honestly don't see a good reason to do this, and I don't think we should.

[jeffberry]

@timdorr: @markerikson has explained the issue we are facing perfectly; we can no longer have a prop named "context". We probably could've picked a more specific name on our part to begin with, but nonetheless, this is a breaking change for us. Let me know if there is anything I can provide to help drive a conclusion.

EDIT: I should add that we are trying to upgrade from react-redux v5 to v7, so I'm not sure if there was some "crack" that we were falling through in v5 that was allowing this to work.

[markerikson]

That one is unfortunately not going to change. We probably should have picked a different name for the "supply your own custom React context instance" prop, but it's too late now - that would require a breaking change on our part. I will admit that I don't think very many people are using that syntax, but semver-wise we'd have to bump a major again.

(Note, however, that that existed as of v6, and has nothing to do with this PR.)

[cmfcmf]

At the same time, similar to how React strips off key and ref so your component never sees them, up until now we have not passed context and store on to the wrapped component

To clarify, "up until now" means <= v5 || >= v7. v6 did pass through the context to the wrapped component.

EDIT: I should add that we are trying to upgrade from react-redux v5 to v7, so I'm not sure if there was some "crack" that we were falling through in v5 that was allowing this to work.

That's good to know. If you were to update to v6, you would face the same issues with the context prop.

---

The React Redux documentation has a section about using the context inside wrapped components to access the store: https://react-redux.js.org/using-react-redux/accessing-store#using-reactreduxcontext-directly
While the approach described there works even without this PR, it doesn't work if you try to use a custom context. That's why connected-react-router tries to read a custom context from props first and only then uses ReactReduxContext:

  const ConnectedRouterWithContext = props => {
    const Context = props.context || ReactReduxContext
    // ...
    return (
      <Context.Consumer>
        {({ store }) => <ConnectedRouter store={store} {...props} />}
      </Context.Consumer>
    )
  }

https://github.com/supasate/connected-react-router/blob/37e90d7a55717046d4265f5832d3f9c8356e7db8/src/ConnectedRouter.js#L105-L117

[markerikson]

@cmfcmf : and tbh, I view the fact that context leaked as a prop in v6 as a bug.

[rickharris]

If the name of the context prop name isn't going to change, please don't revert this. It's possible for "context" to make sense as a prop name that has nothing to do with react context or redux, and this code allows you to pass through a completely unrelated and non-react-context value without trying to use it as react context

react-redux/src/components/connectAdvanced.js

Lines 174 to 176 in 8a1bb58

	return propsContext &&
	propsContext.Consumer &&
	isContextConsumer(<propsContext.Consumer />)