Skip to content
This repository has been archived by the owner on Dec 15, 2018. It is now read-only.

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

Closed
adregan opened this issue Jul 11, 2017 · 6 comments · May be fixed by #212
Closed

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

adregan opened this issue Jul 11, 2017 · 6 comments · May be fixed by #212

Comments

@adregan
Copy link
Contributor

adregan commented Jul 11, 2017

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
Copy link

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
Copy link

jahed commented Aug 27, 2017

#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
Copy link

jahed commented Oct 14, 2017

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:
jahed@8596fa2
jahed@a800b19

@aweary
Copy link
Contributor

aweary commented Oct 23, 2017

@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
Copy link
Contributor

tptee commented Oct 23, 2017

Makes sense 👍

@tptee
Copy link
Contributor

tptee commented Nov 7, 2017

Fixed in v14.1.1

@tptee tptee closed this as completed Nov 7, 2017
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants