Add createSubspaceProvider, mirroring Redux's createProvider.

[conartist6]

Q	A
Fixed Issues?	Fixes #89
Minor: New Feature?	👍
Tests Added + Pass?	Yes

See changed documentation for a description of the changes : )

[mpeyper]

can we make this

export const createSubspaceProvider = (storeKey = 'store', parentStoreKey = storeKey) => {

so that parentStoreKey will default to storeKey if you want a different key from 'store' but the same for both... and for some reason I much prefer arrow functions 🤷‍♂️

[conartist6]

Arrow function, sure. I considered doing that actually. My opinion is that if you want that behavior the argument order should be swapped:

export const createSubspaceProvider = (parentStoreKey = 'store', storeKey = parentStoreKey, ) => {

Actually though any way you slice it someone is going to have to specify a default parameter they don't really care about in order to specify one that they do. Maybe a literate syntax like createProvider().withStoreKey(key) would be best of all?

[mpeyper]

I think I agree about the defaulting order. If the parent store key is changed, you probably want the child store key to be changed too.

The scenarios I see:

1. parent store default, child store default

   const SubspaceProvider = createSubspaceProvider()

2. parent store override, child store override

   const SubspaceProvider = createSubspaceProvider('other')

3. parent store default, child store override

   const SubspaceProvider = createSubspaceProvider('store', 'other')

4. parent store override, child store default

   const SubspaceProvider = createSubspaceProvider('other', 'store')

5. parent store override, child store different override

   const SubspaceProvider = createSubspaceProvider('other1', 'other2')

As you said, in both 3. and 4. one has be be defined as the default key, which kinda sucks.

How about

export const createSubspaceProvider = ({ storeKey = 'store', parentStoreKey = storeKey, childStoreKey = storeKey } = {}) => {

1. parent store default, child store default

   const SubspaceProvider = createSubspaceProvider()

2. parent store override, child store override

   const SubspaceProvider = createSubspaceProvider({ storeKey: 'other' })

3. parent store default, child store override

   const SubspaceProvider = createSubspaceProvider({ childStoreKey: 'other' })

4. parent store override, child store default

   const SubspaceProvider = createSubspaceProvider({ parentStoreKey: 'other' })

5. parent store override, child store different override

   const SubspaceProvider = createSubspaceProvider({ parentStoreKey: 'other1', childStoreKey: 'other2' })

With this, you'd only specify the one you want to override, with storeKey allowing both to be overridden.

Or, possibly, accepting either a string or an object. If a string is provider, it sets both parentStoreKey and childStoreKey to be the string, something like:

const DEFAULT_STORE_KEY = 'store'

const createSubspaceProvider = (storeKey = DEFAULT_STORE_KEY) => {
  const { parentStoreKey = DEFAULT_STORE_KEY, childStoreKey = DEFAULT_STORE_KEY } = typeof(storeKey) === 'string' 
    ? { parentStoreKey: storeKey, childStoreKey: storeKey } 
    : storeKey

  // ...
}

1. parent store default, child store default

   const SubspaceProvider = createSubspaceProvider()

2. parent store override, child store override

   const SubspaceProvider = createSubspaceProvider('other')

3. parent store default, child store override

   const SubspaceProvider = createSubspaceProvider({ childStoreKey: 'other' })

4. parent store override, child store default

   const SubspaceProvider = createSubspaceProvider({ parentStoreKey: 'other' })

5. parent store override, child store different override

   const SubspaceProvider = createSubspaceProvider({ parentStoreKey: 'other1', childStoreKey: 'other2' })

[conartist6]

Yeah I think your last suggestion hits the nail on the head. It's verbose to do the thing that it should be difficult/rare to need to do, and when you're just trying to proxy on a setting that you already configured using Redux createProvider the syntax is actually exactly the same as createProvider's.

Perfect.

[mpeyper]

The only thing that concerns me is the other thing Provider passes down the context, subscriptionShape, which uses the storeKey as part of it's own key.

The reason this concerns me is that I have no idea what it's used for and what the implications are of it not being the same as the storeKey.

[conartist6]

Haha ok I'll find out.

[mpeyper]

Nice work on this. I've left a few comments for discussion, but overall happy with this approach.

As a side note for your intended use case, the way react-redux uses context is changing soon so it might not continue to work going forward, but it's too soon to tell on any of that, so just a heads up.

[conartist6]

Yeah I've read though the threads on context. I think this should be even easier to do once we're just passing around {Provider, Consumer} pairs.

[conartist6]

OK, so the [contextKey]Provider context key is used for ReactRedux Subscriptions which help ensure that parents components have their updates handled before children. I have updated the API and incorporated that context key into the changeset.

[mpeyper]

I'd like to see at least the storeKey changed to childStoreKey and the console.log(..)s removed before merging. The rest is nice-to-haves or just thoughts.

See the comments in the files for more details.

[mpeyper]

I think I would much prefer to be more explicit about what each storeKey is when supplying an object to prevent any confusion.

const DEFAULT_STORE_KEY = 'store'

export const createSubspaceProvider = (storeKey = DEFAULT_STORE_KEY) => {
  const childStoreKey = typeof storeKey === 'object' ? storeKey.childStoreKey || DEFAULT_STORE_KEY : storeKey
  const parentStoreKey = typeof storeKey === 'object' ? storeKey.parentStoreKey || DEFAULT_STORE_KEY : storeKey;

  class SubspaceProvider extends React.PureComponent {
    getChildContext() {
      const makeSubspaceDecorator = (props) => props.subspaceDecorator || subspace(props.mapState, props.namespace)

      return {
        [childStoreKey]: makeSubspaceDecorator(this.props)(this.context[parentStoreKey]),
        [`${childStoreKey}Subscription`]: this.context[`${parentStoreKey}Subscription`],
      }
    }

    render() {
      return Children.only(this.props.children)
    }
}

[mpeyper]

Did you mean to include these?

[conartist6]

oops, nope...

[mpeyper]

I'm starting to wonder if it would be easier to just use react-redux's Provider to inject our subspaced store back into the context instead of trying to replicate how Provider does it. It might make us more resilient to any changes they may make in the future.

I won't insist on changing it to do that as part of this PR as I don't anticipate them changing anytime soon (other than the huge context API changes but we'll likely have to overhaul a bunch of stuff when that happens anyway).

[mpeyper]

wouldn't mind also seeing some cases for the defaulting of the keys when only one is provided, i.e.

• createSubspaceProvider({ childStoreKey })
• createSubspaceProvider({ parentStoreKey })

[mpeyper]

I'm going to close this due to inactivity and the react-redux v6 changes proposed in #103 will make essentially make changing the store key in the legacy context obsolete.