Support query params?

[kimjoar]

This morning I've been playing around with how we could support query params in redux-simple-router (just as a thought experiment).

In history.listen we receive a Location. With pushPath and replacePath we can change the API to accept LocationDescriptors. However, with no way of creating a LocationDescriptor object from a LocationDescriptor string, it's difficult to support query params.

Here's an example of the problem:

pushPath('/foo?a=b')
// store:
{ location: '/foo?a=b' }


pushPath({ pathname: '/foo', query: { a: 'b' } })
// store:
{ location: { pathname: '/foo', query: { a: 'b' } } }


history.push('/foo?a=b')
// store:
{ location: { pathname: '/foo', query: { a: 'b' } } }


<Link to='/foo?a=b'>
// store:
{ location: { pathname: '/foo', query: { a: 'b' } } }

The problem is of course that every time you pushPath you will get the exact LocationDescriptor you pass in into the store, but every time you history.push or <Link> you'll get an object. We then can't reliably support query params. (If you're very strict about always passing in a LocationDescriptorObject you would have query support, but that feels very fragile.)

We can of course use history.createLocation right now, but it's most likely no longer going to be public in history V2.

To support query params we need to be able to create a LocationDescriptor object both from a LocationDescriptor string and from a Location. We could then easily support query params, and we could also deepEqual check them without knowing about query or anything else that's actually part of the LocationDescriptor (which is how we stop cycles, #50). With a createLocationDescriptor (that always returns a LocationDescriptorObject) our code base would actually be simpler, and we would get query support for free.

Okey, so some potential solutions:

Keep the solution we have now

I.e. path and state in store, but change to accepting LocationDescriptor in push and replace. We don't support query params.

Verbatim LocationDescriptor in store

We have to make it very clear that this is either an object or a string. So it's most likely not very usable for people. We "support" query params, iff you're very strict about always passing in a LocationDescriptorObject. Fragile.

Always go through history "before" store

pushPath
-> UPDATE_PATH_INTERNAL action
-> store.subscribe callback
-> history.push
-> history.listen callback
-> UPDATE_PATH action
-> store.subscribe callback where we break cycle

I.e. we don't trigger UPDATE_PATH immediately, but only within the history.listen callback. We would then reliably have a LocationDescriptorObject in the store, and we could therefore reliably support query params.

This is of course a more complex solution.

Conclusion

It was interesting going through the different solutions, but with createLocation going away and no createLocationDescriptor on the horizon I think it's best to just keep our current solution. I don't think it's worth the added complexity.

Maybe someone else has ideas on how to solve this? Or maybe this is another case for keeping history.createLocation public? @jlongster @mjackson @taion @justingreenberg

(Hopefully I haven't overlooked something very obvious 🙈)

[mjackson]

I tried to eliminate the createLocation API in remix-run/history#182, but I don't think we can yet. I'm not even sure that we can in the long run. Just feels like something we should be able to do away with if we have good support for paths/location descriptor objects everywhere. But history 2 will have createLocation methods on history instances. :)

[kimjoar]

Thanks for the input, @mjackson

[justingreenberg]

With a createLocationDescriptor (that always returns a LocationDescriptorObject) our code base would actually be simpler, and we would get query support for free.

i agree 100% now that we know we're not losing createLocation, i feel like it should be easier to track the history api more closely ie syncing with LocationDescriptor object

[kimjoar]

Obvious problem: we don't have access to history in pushPath and replacePath. So fairly sure this requires a bigger change than I initially thought :/

[taion]

IMO the correct solution here is your "always go through history" proposal.

The conversion of location descriptors to locations is properly the responsibility of the history package; this means that you can be confident that things will stay in sync and go through exactly the relevant logic for setting up the "push" settings in history.push, as opposed to trying to re-invent this yourself.

In general it feels wrong to me to store location descriptors at all - they're really argument objects and not meant to be canonical, and it feels weird to me to do the location descriptor -> location conversion more than once (i.e. it should just be done once in history and you should use the result from your listen callback).

[kimjoar]

Great input @taion, thanks!

[kimjoar]

#104 is actually also a point in favour of always going through history.

[taion]

I think you should actually round-trip through the router, FWIW. The location corresponding to the current routing state is not necessarily the history's location - if you use async transitions, the Router location updates after the history one does.

[idolize]

So, this would add support for getting query params from the router state in the redux store, but what about path params?

Ex: getting the id from a /inbox/messages/:id path

I know this is easy to do inside the React view itself with react-router, but I personally have use cases where I want to do things in my Redux reducers based on this information (without having to put logic in my view layer to dispatch actions based on changing react-router props - this seems completely backwards to me).

Is the recommended approach for this to write helper methods to parse the full pathname string and derive the path params by hand? If so, this seems like a very brittle and time-consuming solution as you would essentially have to write a parse method for each possible path param based on a knowledge of the route hierarchy. If the route configuration changes then you'd have to update each of these helper methods as well.

Why can't the state match the lovely props we get in react-router route components?

[jlongster]

I want to do things in my Redux reducers based on this information

If we stored more router state in the app state this still wouldn't help you. You don't have access to any other state in your reducer. It sounds like you want it in the UPDATE_PATH action.

Why can't the state match the lovely props we get in react-router route components?

Because props has all sorts of things which do not belong in the app state; things that are unserializable and not dumb JavaScript objects.

[idolize]

@jlongster Sure, if the UPDATE_PATH action is always guaranteed to fire when the URL changes, and it contains all the relevant information, then I can do the bookkeeping myself. It would in fact be possible for another reducer to read the state of the routing reducer if needed (just call the second reducer with the result of the routing reducer), but you are probably right that it would be preferable for me to manage that myself.

I'm not suggesting one should store every (unserializable) prop from react-router, but the params and query Objects would be extremely useful (in addition to the path String).

[ngthorg]

i am have a example using params. But it (redux-simple-router) not support

function mapStateToProps(state) {
  const { login } = state.router.params

  return {
    user: users[login]
  }
}

export default connect(mapStateToProps)(UserPage)

link example: https://github.com/rackt/redux/blob/master/examples/real-world/containers/UserPage.js#L76

[taion]

Yeah I think especially once we move to the new API, the right place to put this integration is going to be "below" the <Router> in the routing context bit, rather than wrapping the history - solves some async issues and gives you access to the route match, which you can't get by listening to the history.