FormidableLabs / redux-little-router

A tiny router for Redux that lets the URL do the talking.
MIT License
1.04k stars 114 forks source link

Query object is not passed when using the back button #211

Closed adregan closed 6 years ago

adregan commented 7 years ago

If you:

1) navigate to a page with a query param (eg. /products?q=cool) 2) then navigate away from that page (eg. products/12345) 3) then click the browser's back button (eg. products/12345 -> /products?q=cool)

In 1 the ROUTER_LOCATION_CHANGED action includes the query object in the payload; however, in 3 the query object is not available in the payload (but you do have access to the search param).

I have a small change to address this and will be opening a PR shortly.

jakewisse commented 7 years ago

Is this not a duplicate of #165? We've been seeing this on v13.2.0, but haven't tried things out since v14 went out. IIRC, it was pretty obvious that this would be an issue, since the deserialized query property is added to the history's state when navigating with RLR actions, and only retrieved with unpackState(). I never understood the reasoning behind this. It seems that any and all state derived from the raw location should be computed when responding to popstate/whatever change event the history lib. emits.

Just glancing at #197, it looks like this is the exact change that @tptee implemented. 👍 We'll have to upgrade and confirm that things are resolved.

jahed commented 7 years ago

197 didn't fix the bug.

I may be wrong here, but the way the queue is working doesn't look correct.

Glancing through the code, it looks like the usage of isNavigationAction in the reducer is the issue here.

  if (isNavigationAction(action)) {
    return {
      ...state,
      queue: state.queue && state.queue.concat([action.payload])
    };
  }

GO_BACK is considered a navigationAction so its empty payload gets queued. However, unlike other navigation actions, it's never taken back out, so it remains in the queue. Then when an actual navigation action comes along, it picks up the undefined queued payload instead of its own and incorrectly assigns undefined to the query. For future navigations, the entire queue is off by one.

I've applied a simple fix to this, which I've confirmed working with my app with no problems so far:

  if (isNavigationAction(action) && action.payload && action.payload.pathname) {
    return {
      ...state,
      queue: state.queue && state.queue.concat([action.payload])
    };
  }

I haven't gone through the entire codebase to provide a proper, well thought out fix and I can't get the Mocha tests to run on Windows so I'll leave the PR to someone else.

jahed commented 6 years ago

Seems this repo's gotten inactive. I have hacked together a fork in the mean time for those who need it this issue fixed.

You can find my fork here the two fixes: https://github.com/jahed/redux-little-router

The commits that fixed it for me are: https://github.com/jahed/redux-little-router/commit/8596fa2cba9a9199419da4bdce1b2c51a15edad3 https://github.com/jahed/redux-little-router/commit/a800b1959ce86ffa693c190c098f734ec1bd050c

aweary commented 6 years ago

@tptee I think there are two distinct issues here. First, the issue that @jahed outlined where the queue is populated with invalid values since it queues for BareAction which has no payload.

That issue occurs specifically when you dispatch goBack. The issue that @adregan is reporting happens when you use the browser's back button. As far as I can tell, that's because the history callback isn't passing query to locationDidChange.

I'm thinking this can all be resolved by:

  1. Only queuing actions with payloads (IndexedAction, LocationAction)
  2. Passing query to locationDidChange in the history.listen callback.

Do those sound reasonable to you?

tptee commented 6 years ago

Makes sense 👍

tptee commented 6 years ago

Fixed in v14.1.1