Accept objects in push and replace

[taion]

Discussed on Discord a bit, thought I'd get some code in place.

I think this is a bit of a first step toward remix-run/react-router#2186, as described in remix-run/react-router#2186 (comment).

[taion]

The immediate benefit of this will be to enable patterns like

history.push({ ...location, query: nextQuery })

[taion]

Squashed in a couple of test cases showing off the using spreads.

I'm aware this doesn't update the changelog - it takes me a long time to write that sort of thing, so wanted to get feedback on the new functionality before adding a changelog entry.

[taion]

Ah - this is a little tricky because of our semantics with handling search and query. I added some failing test cases for the moment.

[taion]

Well - this resolves it. I don't love this implementation, but I don't hate it either.

For a breaking change, it'd be nicer to just drop the search entirely from the location that listen gives to listeners; this would also make the API a bit cleaner, since it means that the search I get back is exactly the search I pass in.

In practice, though, I think this implementation is good enough.

[mjackson]

I have a local branch that implements this as well; just haven't had time to push it yet. I'll take a look at this on my flight to the UK this weekend. Thanks for all your work here, @taion. :)

[taion]

A couple of things to note - I'd like to resolve this TODO sooner rather than later, but it'd require a (mostly non-breaking) 2.x. Should hold off on actually doing a major version bump to aggregate a few more of these technically breaking changes, though.

Also, my real motivation here isn't just that this API feels "cleaner" in some sense. Concrete things:

• I strongly suspect that supporting mobile routing is going to require a bit more than just the existing state, path, query tuple, and is going to instead need a few extra keys. For this, it's going to be nicer to pass in an object from the start, rather than mucking with the signature on pushState. cc @skevy
• This lets someone implement named routes as a first-class things w/e.g. a useNamedRoutes history enhancer, that allows the user to do e.g. history.push({name: "foo", params: {fooId: 3}}) (and possibly just history.push("foo")). It wouldn't even require a different <Link>, as <Link> could just accept an object for props.to, and pass that to history.push.

[taion]

cc @jlongster

This also opens up the path toward adding a more generic history.transition (or history.update or whatever) that doesn't presuppose action. Might be a better entry point for an integration like redux-simple-router that just wants to forward stuff onto the history; otherwise for completeness I feel like that library ought to have a replacePath (with the current API).

[taion]

On second thought, that might be too generic an API. Probably better for integrations that want to do this to just implement both push and replace hooks.

[mjackson]

The work required for #162 is going to touch a lot of these same points. I think it may be a bit cleaner to do that first.

[taion]

I disagree - I don't think #162 is on-path. The longer-term goal here is to offer a "generic" API to push and replace that better supports paradigms outside of the state/query/path paradigm, which is very focused toward doing path-based routing in browser. One of the goals here is to move the React Router <Link> API to be something like:

<Link to={{path: '/foo', query: {the: 'query'}, state: {the: 'state'}}>

Where the arguments to link.props.to are passed directly to history.push. This then lets users just sub out the history without changing the <Link> component, which is going to be really great for RN support, as well as for "good" userspace named routes support.

[ryanflorence]

Where the arguments to link.props.to are passed directly to history.push

This is appealing.

[mjackson]

I've read the code through a few times, but I'm still not sure what searchBase is or why it's necessary. Is there any way you can make this code more clear?

[taion]

It's for https://github.com/rackt/history/pull/141/files#diff-822f74db4c614a797843213f3d8ad044R71.

Suppose you do history.push({...location, query: nextQuery}). Because useQueries currently appends the queries to the existing search, this will append the contents of the query to the old search.

I don't expect this to be what anyone wants. The ideal solution here is to change the useQueries semantics to overwrite search entirely, but I don't want to make a breaking change yet.

This is a bit of a hacky workaround - just check to see what the "intended" search is. It's ugly, but I couldn't figure out anything better that was non-breaking. I think this will work well enough for most practical use cases, and would rather ship this than hold off on a 2.x release for this change.

[mjackson]

In the test case you linked to, the given query overwrites the pre-existing query string instead of appending to it. Am I missing something?

[taion]

The point of this logic is exactly to allow that overwriting (not exactly overwriting but close).

[taion]

The $searchBase is quite a bit of a hack. The way this works is that if you have a user-pushed location that looks like e.g.

{
  path: '/the/path',
  search: '?a=one',
  query: { the: 'query value' },
}

We end up getting producing something that looks like:

{
  path: '/the/path',
  search: '?a=one&the=query+value',
  query: { the: 'query value' },
  '$searchBase': {
    searchBase: '?a=one',
    search: '?a=one&the=query+value',
  }
}

Suppose we want to follow the pattern above and do something like:

history.push({
  ...location,
  query: { other: 'query value' },
})

The object that actually gets pushed looks like:

{
  path: '/the/path',
  search: '?a=one&the=query+value',
  '$searchBase': {
    searchBase: '?a=one',
    search: '?a=one&the=query+value',
  }
  query: { other: 'query value' },
}

Since the search matches history['$searchBase'].search, we replace it with history['$searchBase'].searchBase, and in a roundabout way end up with search as ?a=one&other=query+value, which (given the current append semantics) is what we expect.

On the other hand, if the user does something like:

history.push({
  ...location,
  search: '?b=two',
  query: { other: 'query value' },
})

Then we end up with search as ?b=two&other=query+value.

There are edge cases here, but I expect that people are very unlikely to run into them in practice. I intend this as a hack that we remove ASAP, but one that is necessary if we don't want to break backward compat per the current "when there is already an existing search" test case.

The idea would be something like releasing this as v1.14.0 and taking a bit more time to gather stuff like the potential query-string for qs replacement before cutting v2.x. The TODO below is sort of in lieu of further documentation, because I didn't want to imply that this was deservedly complicated code that we should keep - it isn't anything of the sort.

[mjackson]

Awesome, thanks for the detailed explanation.

I think ultimately useQueries should probably just ignore any existing search value and we should alter that test to make sure the existing query string is overwritten. I think this is a lot clearer in the object-based API where everything is in the same argument. I understand you'd like to keep backwards compat for now, but let's def drop in 2.x.

[mjackson]

Thanks, @taion! Let's be sure to follow up with a note in CHANGES.md.

[taion]

Ah crud, I should also have cleaned up the commits on this branch.

I want to drop the hack in useQueries ASAP - entirely just a question of what else we want to break first. I'm trying to track possible things on #164... possibly at least all the things under "minor", just to get those out of the way.