Slashes in property names are treated as path delimiters in path objects for nested locations

[anndroow]

Hi,

In the documentation, there are three examples of using store.attachReducers():

store.attachReducers({
    some: {
        path: {
            to: {
                dynamicReducer
            }
        }
    }
} )

store.attachReducers({ 'some.path.to': { dynamicReducer } } } })

store.attachReducers({ 'some/path/to': { dynamicReducer } } } })

The last two examples use a string to specify the path and in the last one, slash is used as the delimiter. But when using the first approach to describe the path of a nested location, it seems like slashes in the property names of the path object are treated as path delimiters as well. The thing is that we're using slashes in some of the property names in the state tree.

Here's an example of what's happening for us:

store.attachReducers({
    routes: {
        '/route1/section1': dynamicReducer
    }
} )

...will end up like this:

{
    routes: {
        '': {
            route1/section1': {...}
        }
    }
}

[mpeyper]

Hi @anndroow,

I can see how this might be causing you some headaches... Sorry about that!

The problem here is how we store the structure internally. Basically, the object and / syntax are helpers for us to get to the . format. You can see how we convert them here. So for your example, it should be no different than calling attachReducers like so

store.attachReducers({
  'routes..route1.section1': dynamicReducer
} )

Obviously the double . is not what was intended here. I've also just noticed a bug on this line where it's only going to replace the first occurrence of the slash and not all of them (although I think this would actually make your particular scenario worse).

So what can we do about it?

The option that comes to mind are to change the logic to only consider slashes when the whole structure is slash delimited paths. The logic for dot delimited paths would probably change to be consistent with the slash version and then the logic for the object structure would not consider either of them when resolving the structure.

Here are some examples:

store.attachReducers({
  'routes.route1.section1': dynamicReducer
} )

becomes:

{
  routes: {
    route1: {
      section1: {...}
    }
  }
}

store.attachReducers({
  'routes/route1/section1': dynamicReducer
} )

becomes:

{
  routes: {
    route1: {
      section1: {...}
    }
  }
}

store.attachReducers({
  routes: {
    route1: {
      section1: dynamicReducer
    }
  }
} )

becomes:

{
  routes: {
    route1: {
      section1: {...}
    }
  }
}

store.attachReducers({
  'routes./route1/section1': dynamicReducer
} )

becomes:

{
  routes: {
    '/route1/section1': {...}
  }
}

store.attachReducers({
  routes: {
    '/route1/section1': dynamicReducer
  }
} )

becomes:

{
  routes: {
    '/route1/section1': {...}
  }
}

I would consider this a breaking change and would need to be a major version bump.

My biggest concern with this is that there might be some unexpected behaviour when calls are made with structures that themselves have dynamic shapes.

For example, consider this call:

store.attachReducers({
  'routes/route1/section1': dynamicReducer1
} )

The result of this call would change dramatically if this second reducer was optionally attached at the same time:

store.attachReducers({
  'routes/route1/section1': dynamicReducer1,
  admin: {
    'routes/route2/section2': dynamicReducer2,
  }
} )

This isn't a deal breaker on a change like this for me, but it would need to be considered whether which would be the more common use case and be carefully documented what the behaviour is.

As always, I'm open to anyone's ideas on these types of things.

[anndroow]

Hi,

Thanks for the quick response. Your examples of the fixes for this issue make sense for me and I understand that it needs to be a new major for these changes.

Regarding the last concern, maybe introducing a configuration for this could be something so you could choose the behavior you want.

Another thing related to this, when using withReducer(), is it supposed to not be able to use the object approach to describe the path? Looks like just withReducer(reducer, "some/path") is supported right now :)

Also, I have another question regarding the usage of withReducer, but I'll add another issue for that to not go off-topic :)

Thanks again for the support.

Cheers!

[jpeyper]

Would escaping the identifier be a workable solution for you?

store.attachReducers({
    routes: {
        [escapeIdentifier('/route1/section1')]: dynamicReducer
    }
} )

[mpeyper]

when using withReducer(), is it supposed to not be able to use the object approach to describe the path?

That is correct.

Looks like just withReducer(reducer, "some/path") is supported right now

I think from #11, only withReducer(reducer, "someIdentifier") is currently supported (i.e. using / or . in the identifier causes an error).

Happy to review a PR adding object syntax to it.

[mpeyper]

@jpeyper I quite like that suggestion. It gives control back to the developer for when to allow nesting and when not to. It's also backwards compatible 😃

@anndroow Does this work for you?