Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Preserving query params across route changes #13

Closed
tscizzle opened this issue Sep 4, 2017 · 13 comments
Closed

Preserving query params across route changes #13

tscizzle opened this issue Sep 4, 2017 · 13 comments

Comments

@tscizzle
Copy link

tscizzle commented Sep 4, 2017

React Router's history.push('/thing') squashes the query params (what it calls location.search).

One can do history.push({pathname: '/thing', search: location.search}), to preserve the location.search, but an action could be dispatched just before history.push which changes the query params, so the location.search that history.push is using is now out of date, and it again squashes the query params.

It's making it really tough for me to use this package, which is otherwise dope.

Any ideas?

Nice package, btw!

@dmitkataev
Copy link

@tscizzle what version of React Router do you use?

@dmitkataev
Copy link

dmitkataev commented Sep 4, 2017

i've just tested your issue on v3 (router v3 = history v3):
browserHistory.push({ pathname: '/test', search: browserHistory.getCurrentLocation().search, });
And it works well. Pay attention if you use v3 then you don't have history.location - you have history.getCurrentLocation()

By the way, why do you use direct history.push and not router.push?

@Treora
Copy link
Owner

Treora commented Sep 4, 2017

I have not sufficiently tried combining this module with a router, but my understanding is that things should work as long as the router changes only the path; you may have to make yourself a small helper function to ensure it does not change anything else. (like @dmitkataev's example, I would read from history.location/history.getCurrentLocation() rather than window.location)

function push(pathname) {
    history.push({pathname, search: history.location.search, hash: history.location.hash})
}

Or, using object-rest-spread syntax:

function push(pathname) { history.push({...history.location, pathname}) }

... but an action could be dispatched just before history.push which changes the query params, so the location.search that history.push is using is now out of date, and it again squashes the query params.

I do not understand how this problem could happen. If the update of the path is atomic (no pauses between reading and setting location), that dispatched action's effect on the query params should either happen before or after the update of the path, and things should go just fine. If you are able to create a simple example that proofs me wrong, please share it. It could well be that I have made wrong assumptions about the implementation of history.

@tscizzle
Copy link
Author

tscizzle commented Sep 5, 2017

@dmitkataev I'm using v4

@Treora Consider a component that has location and history injected into its props by withRouter. In a click handler, it grabs location and history from props, and location.search is A at the time of click.

import { withRouter } from 'react-router-dom';

let MyComponent = ({ someVal, setStateToSomeVal, location, history }) => {
  const handleClick = () => {
    setStateToSomeVal(someVal);
    history.push({
      pathname: '/nextPathname',
      search: location.search, // location.search is A, `setStateToSomeVal` setting the query param to B is not reflected here
    });
  };

  return <button onClick={handleClick}> Click me </button>
}

MyComponent = withRouter(MyComponent)

After clicked, setStateToSomeVal results in the redux state (that is synced with the URL query param) being changed to B, so the URL query param follows and becomes B. But history.push hasn't run yet, so then it runs, and search.location has the param's value as A, so it overwrites the URL param back to A (instead of leaving it as B).

If, in the click handler, after setStateToSomeVal sets the redux state and URL query param to B, I could use the updated location.search (i.e. B) as the arg to history.push then that'd great. But I'm not sure how to get the updated one, as opposed to the stale one, as in the case in the code above.

(Note that it yields the same situation when using history.location.search. I am using your helper, which I like a lot.)

@tscizzle
Copy link
Author

tscizzle commented Sep 5, 2017

@dmitkataev Also, using direct history.push because I believe that's how it's supposed to be done when using withRouter https://reacttraining.com/react-router/web/api/withRouter.

What is this router.push you speak of?

@dmitkataev
Copy link

@tscizzle you can get an access to a router object via context, defining a static variable in your component class, f.e.
static contextTypes = { router: React.PropTypes.object, };
and then you can do inside a component method this.props.router.push({query: { a: 1, b: 2 }). But now it's recommended to use withRouter, so your way is correct.
The interesting thing in your code is setStateToSomeVal. You said it's synced with URL, but how?

@Pringels
Copy link
Contributor

Pringels commented Sep 5, 2017

Partially related to this issue is the fact that the hash will also get squashed.
Perhaps just adding a the current location's hash might fix this? EG:

? history.replace({pathname: location.pathname, search: newLocationSearchString, hash: location.hash})
: history.push({pathname: location.pathname, search: newLocationSearchString, hash: location.hash})

@dmitkataev
Copy link

dmitkataev commented Sep 5, 2017

@Pringels you are right!

@Pringels
Copy link
Contributor

Pringels commented Sep 5, 2017

Opened a PR: #14

@Treora please take a look when you have a moment

@Treora
Copy link
Owner

Treora commented Sep 5, 2017

@tscizzle Thanks for the code example. I understand how it goes wrong when reading the location property, since it will be outdated by the time it will be read. However, you mention the problem also occurs when using history.location, which I would have expected to remain up to date. Are you certain there was not another issue hidden somewhere?

  const handleClick = () => {
    setStateToSomeVal(someVal);
    // Do these two really output different values?:
    console.log(window.location.search);
    console.log(history.location.search);
  }

If they are different, could the problem possibly be that ReactQuerySync is using a different history instance than react router? (see #9) Are you using (at least) v0.1.6, and did you pass it the history instance?

@tscizzle
Copy link
Author

tscizzle commented Sep 6, 2017

Ok, I passed ReactQuerySync my history instance and it works!

To summarize, for anyone following along, or coming across this via search:

I was using react-router and was not explicitly using the history library. I used react-router's BrowserRouter like so:

import { BrowserRouter } 'react-router-dom'
...
  <BrowserRouter>
    ...
  </BrowserRouter>

@Treora asked if I passed ReactQuerySync the history instance, and I had not (I didn't even know if I had access to any history instance since BrowserRouter brings its own history with it for free.)

So I made the following changes:

  1. use history
  2. pass a history object to my Router, which I changed to a plain Router instead of BrowserRouter
  3. pass that same history object to my ReduxQuerySync initialization

Example of each of those things:

// src/myHistory.js
import createHistory from 'history/createBrowserHistory';
const history = createHistory();
export default history;

// whichever component your router lives
import history from './myHistory';
...
<Router history={history}>
  ...
</Router>

// src/myReduxQuerySync.js
import history from './myHistory';
...
const enhancer = ReduxQuerySync.enhancer({
  params: {
    ...
  },
  initialTruth: 'location',
  history,
});
...

@Treora
Copy link
Owner

Treora commented Sep 7, 2017

@tscizzle thanks for sharing! It is unfortunate that this needs such tricks to work.

It would have been much easier if the history instance would not have to be shared, but that would require changes in the history module. It would have to behave like a polyfill (i.e. provide a shared single instance globally). Or the browser behaviour itself would have to be redefined, to also emit the popstate event after use of history.pushState/replaceState, which I think would solve the core problem.

I guess the best we can do is just provide instructions for how to set this up correctly.

@Treora
Copy link
Owner

Treora commented Sep 16, 2017

Added a small hint to the readme with a link to your explanation (commit 96784cb). Closing this issue.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants