Add a LegacyProvider component

[timdorr]

This avoids polluting Provider with StrictMode-incompatible stuff. Hopefully, this makes working with outdated libraries easier.

This is per Dan's comment

[gaearon]

It should probably warn that this is deprecated, exists strictly as a migration step, and will break in the future. And that this makes your app slower.

[markerikson]

I don't think I like this idea, for several reasons:

• Legacy context will go away eventually
• React-Redux v5 still works fine, except potentially in concurrent mode
• Accessing the store out of context has never been part of our public API
• There's been several React issues filed where using legacy context and new context together caused problems
• People would need to slap another wrapper component around the entire tree
• The potential perf issues that Dan described

The community has done some neat stuff based on unofficially accessing the store out of context. I don't want to officially support it, but I don't want to prevent people from doing that either.

This is one of the reasons why I chose to put the entire store into new context, rather than just the dispatch method. The primary goal is to use new context to make the latest store state accessible, but if someone really wants to grab the store out of the context consumer, they can. That's also why we're exporting the default context object.

If we're going to do a major version based on switching from legacy context to new context, we should just move on. If people want to stick with v5, they can.

At most, we could give a semi-recommended pattern for "If you really want to, here's how you could get access to the store, but be aware this could break at any time" (which basically amounts to reusing the exported context object).

[mpeyper]

redux-subspace and redux-dynostore author/maintainer here.

Whilst this may work as a temporary measure to keep compatibility when moving to new context, I do not think it should be the end goal for supporting our use cases. What we do in both my libraries is not possible with just a getState and dispatch function as we store additional fields and utility functions on the store itself. This has been a useful pattern as:

a) It's already being passed to all the places we want to use it
b) It's relatively hidden to the rest of the ecosystem as anything outside our library only knows about getState and dispatch
c) Store enhancers provide a really convenient API for adding them

That said, I could envision libraries like mine providing a replacement provider to inject the store into an alternative context to get access to the store instead, e.g.

// Context.js
import React from 'react'

export const Context = React.createContext()

// Provider.js
import React from 'react'
import { Provider as ReduxProvider } from 'react-redux'
import { Context } from './Context'

export const Provider = ({ store, children, ...props }) => {
  return (
    <ReduxProvider {...props} store={store}>
      <Context.Provider value={store}
        {children}
      </Context.Provider>
    </ReduxProvider>
  )
}

// SubspaceProvider.js - the interception component
import React from 'react'
import { subspace } from 'redux-subspace' // does the _real_ work
import { Provider } from './Provider'
import { Context } from './Context'

// over simplified and performance implications not taken into account at all
export const SubspaceProvider = ({ mapState, namespace, children }) => {
  return (
    <Context.Consumer>
      {(store) => (
        <Provider store={subspace(mapState, namespace)(store)}>
          {children}
        </Provider>
      )}
    </Context.Consumer>
}

This would work fine as long as there was only a single library using this pattern. As soon as both redux-subspace and redux-dynostore (which are designed to be used together) are required, we'd need to provide some mechanism for passing what the wrapped provider so that one could wrap the redux provider and the other wrap that provider.

[markerikson]

In the case of connected-react-router, it wants to subscribe to the store to read out routing state:

      this.unsubscribe = context.store.subscribe(() => {
        // Extract store's location
        const {
          pathname: pathnameInStore,
          search: searchInStore,
          hash: hashInStore,
        } = toJS(getIn(context.store.getState(), ['router', 'location']))
        // Extract history's location
        const {
          pathname: pathnameInHistory,
          search: searchInHistory,
          hash: hashInHistory,
        } = props.history.location

        // If we do time travelling, the location in store is changed but location in history is not changed
        if (pathnameInHistory !== pathnameInStore || searchInHistory !== searchInStore || hashInHistory !== hashInStore) {
          this.inTimeTravelling = true
          // Update history's location to match store's location
          props.history.push({
            pathname: pathnameInStore,
            search: searchInStore,
            hash: hashInStore,
          })
        }
})

Although looking at this, I'm curious why it doesn't just use connect instead.

[mpeyper]

For the record, I'm willing to accept that my use case is fairly niche, so I do understand if not supporting it to save bloating the library for everyone else might actually be the preferred approach here.

[markerikson]

@mpeyper : I've seen other libraries that do the same kind of thing, so you're not the only one :) Just the first example that comes to mind.

Off the top of your head, does accessing the store via the ReactReduxContext instance work for your situation?

[mpeyper]

Yes, I believe it would, assuming it is just the same store that is passed to the Provider (which it looks like it is from a quick look at the code).

Although, we would need access to ReactReduxContext.Consumer to use it if I'm not mistaken.

Edit: it appears it is already being exported too.

[markerikson]

Right, we export the entire context object - see Context.js and index.js.

And yes, our <Provider> puts the entire store object into context, even though connect really only needs access to dispatch.

Both of those are to specifically (unofficially) support this use case.

[mpeyper]

Your tweet made it sound like it was going away in favour of this PR. If that's not the case, great!

[markerikson]

Ah, sorry if I gave that impression.

No, the discussion in #1083 was how to handle libs that currently expect to access the store out of legacy context. Dan proposed adding this in addition to the new context usage, Tim filed a PR, and I'm saying I don't think it's a good idea. But, I wanted to get other eyes on the proposal, especially for lib authors like you.

I guess I'm primarily interested in confirming that exposing the store through new context will be enough of an "unofficial API" to allow libs to keep doing whatever it is they do now.

[timdorr]

OK, I'm agreed on this not being the right approach. Figured it was worth a quick PR. Thanks for everyone's input!