dynamic HOC breaks if the identifier uses / or . characters when subspaced [@redux-dynostore/core, @redux-dynostore/react-redux, @redux-dynostore/react-redux-subspace, @redux-dynostore/redux-subspace]

[mpeyper]

Is it a bug, feature request or question?

Bug

Which package(s) does this involve?

@redux-dynostore/core
@redux-dynostore/react-redux
@redux-dynostore/react-redux-subspace
@redux-dynostore/redux-subspace

Input Code

This issue is duplicating ioof-holdings/redux-dynamic-reducer#11 from the old library.

export default dynamic('test/component', addReducer(myReducer), subspaced())(MyComponent)

Here is a sandbox demonstrating the issue

Expected Behavior

The component is able to mount and map it's state correctly

Current Behavior

Upon mounting, the following error is thrown:

TypeError
mapState must not return undefined.

This appears to be caused by redux-dynostore adding the reducer with the additional nesting locations, but the subspace wrapper not mapping the state in the same way, i.e.:

the reducer is attacher to

{
  "test": {
    "component": myReducer
  }
}

but the subspace is looking for

{
  "test/component": myState
}

Possible Solution

I can see 2 options:

1. Change the subspace state resolution to honor the additional levels specified in the identifier
2. Escape the characters so that the additional layers are not created

Personally, I prefer option 2 as the original intention of the HOCs was to create the state structure based component hierarchy, so if a test layer was intended then a Test component should be the one adding the layer and MyComponent would just use the component identifier. This also plays nicely with the proposed solution for #2.

Your Setup

package	version(s)
redux-dynostore	1.0.0

[mpeyper]

Once #13 is merged it would be very simple to do option 2.

However, I made a comment in #12 about possibly concating instance identifiers with the base identifiers. As a follow up thought to that, it might be cool if they were separated by a / (or .) to add a grouping around the instance state, but this would require option 1 to be implemented instead.

e.g.

const BaseComponent = dynamic('counter', addReducer(counterReducer), subspaced())(Counter)

const Counter1 = BaseComponent.createInstance('one')
const Counter2 = BaseComponent.createInstance('two')

and when both counters are mounted the resulting state would be something like:

{
  "counter": {
    "one": 0,
    "two": 0
  }
}

[jpeyper]

I would say option 2 for now.. concating the identifier is a breaking change and might require more thought on what impact it would have is someone was relying on being able to completely change the identifier.

[mpeyper]

I guess we can always do option 2 now and maybe option 1 in a future release.

[mpeyper]

So I had a go at option 2 and there was an unexpected consequence of just escaping the identifier. I works for the component finding it's state for the correct subspace, but the actions still have the escaped tokens in them (makes readability a nightmare). Trying to have it so that everything was escaped and unescaped at the right points led me to a dead end where the subspace could only give me the unescaped version and I couldn't reliably escape it again because I'd break legit subspaces.

So I tried option 1 and it turns out it's a 1 liner. I haven't done the out prefixing of createInstance calls, but it would Just Work™️ if we wanted to do it in a major release some time.

I still need to clean it up a bit but I think I'll continue in that direction.

You can see what I've done so far for option 1 and option 2 if you want.

[mpeyper]

Closing. Please see #484 for details.