Support ImmutableJS Map as the state

[zavelevsky]

Although it makes sense to use ImmutableJS Map object as your state object - it currently can't be done because the reducer of redux-simple-router uses simple Object.assign.
Please consider supporting it by allowing to pass a custom reducer, or a reducer's internal state creator, such as (state, obj) => newState.

[mxstbr]

👍 I don't want to keep people from being able to use ImmutableJS if they want to with my boilerplate, would be nice if support for this added! (See react-boilerplate/react-boilerplate#6)

[neverfox]

+1 for a custom state access function. That seems to be the idiomatic way this is handled in other redux libraries that provide their own reducer.

[jlongster]

We support a custom state accessor function in https://github.com/rackt/redux-simple-router#syncreduxandrouterhistory-store-selectrouterstate

[neverfox]

Oh, ha. I just took the OP's issue at face value and didn't check that. I wonder what the problem is then. That should be all you need to support any immutable library.

[jlongster]

I'm considering closing this unless the OP can describe further what's wrong. I know several people that use Immutable.js just fine with this.

[zavelevsky]

Last I checked you return the result of Object.assign from the reducer.
This of course returns an object and not the original Map.
On Jan 1, 2016 1:52 AM, "James Long" notifications@github.com wrote:

I'm considering closing this unless the OP can describe further what's
wrong. I know several people that use Immutable.js just fine with this.

—
Reply to this email directly or view it on GitHub
#140 (comment)
.

[zavelevsky]

Original type, which is Map.
On Jan 1, 2016 7:27 AM, "Doron Zavelevsky" zavelevsky@gmail.com wrote:

Last I checked you return the result of Object.assign from the reducer.
This of course returns an object and not the original Map.
On Jan 1, 2016 1:52 AM, "James Long" notifications@github.com wrote:

I'm considering closing this unless the OP can describe further what's
wrong. I know several people that use Immutable.js just fine with this.

—
Reply to this email directly or view it on GitHub
#140 (comment)
.

[neverfox]

An original Map of what? Can you be more specific about what you're trying to accomplish?

[zavelevsky]

Sorry, I'm limited to answering by email at the moment.
As I recall, the lib's reducer returns the result of Object.assign,
therefore an object.
If my state is an ImmutableJS Map then after the reducer is called, what I
get is no longer an instance of a Map but a simple object.
Tjis might have changed since last I checked, but I cannot check at the
moment. Just wanted to clarify so the issue won't be closed prematurely.
On Jan 1, 2016 7:49 AM, "Roman Pearah" notifications@github.com wrote:

An original Map of what?

—
Reply to this email directly or view it on GitHub
#140 (comment)
.

[neverfox]

So what you're asking for is not just for the reducer to be able to get it's necessary sub-state out via a customer selector, but for the ability to tell the reducer how to write the new state back out, e.g. via fromJS()? Is that just because you want to be able to consistently access state values with the ImmutableJS API, e.g. state.getIn(['routing', 'path']) instead of state.get('routing').path?

[neverfox]

Could you not then just do something like:

syncReduxAndRouter(history, store, state => state.get('routing').toJS())

...

combineReducers({
  routing: (state, action) => routeReducer(state, action).fromJS()
})

[zavelevsky]

Specifically, combineReducers doesn't support immutablejs for the same
reason - it assumes the root of your state is a simple object. But yes - I
can wrap the reducer and import its result. It's not very efficient but the
routing subtree is very small.
I'd rather pass my own state setting function.
On Jan 1, 2016 9:10 AM, "Roman Pearah" notifications@github.com wrote:

Could you not then just do something like:

syncReduxAndRouter(history, store, state => state.get('routing').toJS())

...

combindReducers({
routing: (state, action) => routeReducer(state, action).fromJS()
})

—
Reply to this email directly or view it on GitHub
#140 (comment)
.

[neverfox]

Specifically, combineReducers doesn't support immutablejs for the same
reason - it assumes the root of your state is a simple object.

True, but I assumed that if you're doing what you seem to be doing, then you're already using one of the combineReducers mods from some project like redux-immutablejs or redux-immutablejs, or your own.

So, you want to be able to tell redux-simple-router how to set values in state, right? So instead of:

return { location }

it would be

return setLocation(location)

with that function being provided as:

function setLocation (location) { return Immutable.Map({ location }) }

I don't really see the advantage over fromJS when it requires a code change to the library. If the reducer were more complex, then I imagine you'd still end up just writing a state setting function that uses fromJS() since you couldn't easily account for all the different ways state might be altered (e.g. some places might need set and others merge or updateIn).

[zavelevsky]

OK, I'm convinced. Thanks, as far as I'm concerned the issue is closed.
Keep up the good work!
On Jan 1, 2016 10:20 AM, "Roman Pearah" notifications@github.com wrote:

Specifically, combineReducers doesn't support immutablejs for the same
reason - it assumes the root of your state is a simple object.

True, but I assumed that if you're doing what you seem to be doing, then
you're already using one of the combineReducers mods from some project
like redux-immutablejs or redux-immutablejs, or your own.

So, you want to be able to tell redux-simple-router how to set values
in state, right? So instead of:

return { location }

it would be

return setLocation(location)

with that function being provided as:

function setLocation (location) { return Immutable.Map({ location }) }

I don't really see the advantage over fromJS when it requires a code
change to the library. If the reducer were more complex, then I imagine
you'd still end up just writing a state setting function that uses fromJS()
since you couldn't easily account for all the different ways state might be
altered (e.g. some places might need set and others merge or updateIn).

—
Reply to this email directly or view it on GitHub
#140 (comment)
.

[stepankuzmin]

So there is no way to use redux-simple-router@2.0.3 with immutablejs?

[timdorr]

Just write your own reducer. Ours is very simple, so you just modify it to store an immutable object.

[stepankuzmin]

Thanks for reply, @timdorr! I've made simple immutable reducer:

import { UPDATE_LOCATION } from 'redux-simple-router'

const initialState = new Immutable.Map({
  location: undefined
})

export default function routeReducer(state = initialState, { type, location }) {
  if (type !== UPDATE_LOCATION) {
    return state
  }

  return state.merge({ location })
}

It works well, but when I apply middleware.listenForReplays(store) I'm getting Uncaught TypeError: Cannot read property 'location' of undefined. Is this approach compliant with redux-devtools?

[timdorr]

Most likely not. The listenForReplays stuff would need to be redone to support Immutable as well.

[stepankuzmin]

Oh, I see. So I'm going back to v1.x :)

[jlongster]

All we need is the location from the state, so we could take a selector getLocationState instead of getRouterState which returns a JS location object. Immutable users would get the location state (an immutable object) and call toJS on it. If you wanted to avoid this perf hit, you could only call listenForReplays in development, as it's only needed in development mode.

[SimenB]

If you wanted to avoid this perf hit, you could only call listenForReplays in development, as it's only needed in development mode.

I know this because I followed the 2.0 issue, but from the readme it's not really clear IMO, it's just a small comment above the call. What if it's wrapped in a if (process.env.NODE_ENV === 'development') check to make it more explicit? People copy&paste examples all the time, it might get left there even when it's not needed

[jlongster]

That's probably not a bad idea. That wouldn't break anything else, right @timdorr @taion ?

[taion]

I'm a little confused, actually. What was the benefit of 0529dea, when we know the only key in the routing tree is the location?

Also, you should be aware that history locations are not (yet) strictly immutable.

I mean, in some sense the location object coming from history should be treated as being opaque, IMO, and converting it deeply into an ImmutableJS map feels very, very odd to me. I'm +1 on getLocationState if it would make things easier, though.

[timdorr]

@taion A reducer should always return a copy of the original state in the state object. It doesn't know if it's being compose with other reducers, so without that it may be blowing away the work of others. I use reduceReducers in my projects rather than combineReducers, which is unfortunately a common pattern for Redux and changes the understanding most people have of what a reducer actually is.

[taion]

Interesting. Thanks. Anyway, it seems like getLocationState would be the correct approach, then (although I still think you ought not convert location objects to immutable maps).

And yeah, definitely should document that listening for replays is a dev-only concern.

[timdorr]

To put it in code for others' sake, it's a bit like doing this:

[1, 2, 3, 4].reduce((prev, next) => return next + 1, 0)

If you ignore the previous value in a reducer, there's not really a reason for doing a reduce, is there? 😄

[taion]

It's a subset. You can implement a last element operation as a reducer. It's just a question of what's exposed to you.

[timdorr]

As to the dev flag point, I definitely like that. It potentially means less hassle for users when they go to production. As a bonus, we save some bytes on a minified bundle.

[taion]

You mean automatically make that part a no-op in production? Or just add it to the example?

[timdorr]

Yeah, just wrap this in a dev flag.

[stepankuzmin]

Hi @timdorr! I've made a PR #218 with getLocationState instead of getRouterState. Is this approach correct?

[taion]

@timdorr Is that conventional for Redux DevTools though? The docs there suggest handling that part in user space: https://github.com/gaearon/redux-devtools#exclude-devtools-from-production-builds.

[timdorr]

I think that's mainly because Dev Tools is, by nature, entirely a development-only thing. The whole module would be a giant no-op otherwise, which would be quite silly. We have some developer-y bits included in ours which have to get included in the full build regardless of its context, so it's more sensible for us to manage that. It's also impossible for them to minify that out when not using it.