Proposal: Make search friendlier

[pshrmn]

The Problem

Currently, the location object uses pathname, search, and hash strings to create URIs. This makes the job really easy for history, but causes a lot of consternation

In history, v2/3, there is built-in (through enhancers) support for query parsing. The biggest issues with that is that 1. you end up with redundant data with the search and query properties and 2. you end up bundling query parsing code that may not even do what the user wants it to do.

The Proposal

What I am proposing is that the search property of a location can be anything you want. It could be a string, or an object, or a function for all we care. The only important thing is that your history object knows how to derive a string from it for the URI. To do this, the history constructors should accept a new option: stringify.

const history = createBrowserHistory({
  stringify: qs.stringify
})

When history calls createPath to create the URI that will be displayed in the address bar, instead of calling:

pathname + search + hash

it would call

pathname + stringify(search) + hash

Of course, the search value should be consistent, so we will also need a way to create a location object with a URI that has the desired search property for the user. Thus, we should also add a parse option to the constructor.

const history = createBrowserHistory({
  stringify: qs.stringify,
  parse: qs.parse
})

Basically, parsePath would be updated to call parse on the portion of the path that begins with ? through to the end of the string.

search = parse(pathname.substr(searchIndex))

Default stringify and parse functions would just return the values that they pass, so by default search would expect to be a string.

Cons

• This makes history more complicated.
• history doesn't really care about anything but how to turn location objects into URI strings and how to turn URI strings into location objects. history is predominantly a routing package, and query strings are not important for routing, so representing them as strings is good enough.
• Despite all of the complaints and the code being open source, no one has submitted a PR with their solution to fix this. Makes you wonder how big of an issue this really is...

Pros

• Convenience for developers
• A dramatic reduction in requests for/complaints about this in the React Router issues.
• As long as we can parse and stringify it, who cares what the search property is?

While I have advocated that this isn't necessary, complaints keep rolling in, so I thought that someone should make a proposal for a solution to this. I should be able to submit a PR for this soon because the changes to the code are pretty minor. A number of tests will need to be added in order to verify that this actually works as expected, but I think that it should be fairly easy to implement.

[mjackson]

I'm not inclined to change the API here. Having a search string gives us a number of advantages, many of which you've already listed. Another one is that 2 search strings are easy to compare (so e.g. we can know if the search string changed).

It sounds like what we really need is a wiki/doc page that explains the issue that we can just link to w/out the need to explain/comment every time. Don't we already have one?

[timdorr]

Could we instead bring back useQueries to enhance the history instance to to do this, rather than bake it into the core API?

[mjackson]

I'd like to avoid creating any "history enhancers". That was a bad API decision because there are only about 3 or 4 enhancers we'll ever create. And trying to account for query parse/stringify on the entire history API leads to a bunch of code which most people will never need.

I'm trying to shed responsibilities and let people make decisions for themselves, not take on more.

[pshrmn]

I have a working (I think) implementation that you can see the changes required for it here: master...pshrmn:search-is-whatever. Only one breaking change, and that was out of convenience not necessity. You'll probably want to use the split view because git doesn't do a great job matching the actual changes in the unified view. Not going to PR it, but just throwing it out there.

@mjackson Alternatively, any thoughts on rendering a component that adds a query property to the location object
like this? https://gist.github.com/pshrmn/d0e888dae58856a95c87a80f730d61fc I don't think that that should break anything and would make the parsed query object available throughout the React application.

Anyways, if you're taking a look at history things, I'm much more interested in #472 and #465.

[odigity]

const history = createBrowserHistory({
stringify: qs.stringify,
parse: qs.parse
})

As a non-expert, I see no obvious downsides to this improvement. My only comment is my OCD would prefer more consistency between the config keys, such as query_serializer and query_deserializer, but that's merely taste, and your version is less verbose.

Cons

This makes history more complicated.

Again, I speak in ignorance of the code, but it seems like this should only require a few additional lines of code.

history doesn't really care about anything but how to turn location objects into URI strings and how to turn URI strings into location objects. history is predominantly a routing package, and query strings are not important for routing, so representing them as strings is good enough.

That's an opinion that others may not share. I personally have no intention of routing by query params at this time (only accessing them in components and setting them on links), but others may. Still, that can be treated as a separate additional problem, probably more suited for an add-on. Once the serializers hook is standard, someone else can build on it to add query-based routing if they choose.

Despite all of the complaints and the code being open source, no one has submitted a PR with their solution to fix this. Makes you wonder how big of an issue this really is...

I'm new to RR4, but from what I gather in the comments for #478, this was a solved problem in R2/3, and R4 is brand new, and the last month or so people have been focused on merely getting maintainers to recognize the problem, before which a PR would be pointless. Now that the issue is being treated as legit, I'd put money on a PR appearing soon (heck, even I'm tempted, and I don't know jack) - or would have, had @pshrmn not already started on a solution in a fork. (Great job, I appreciate it!)

I'm not inclined to change the API here. Having a search string gives us a number of advantages, many of which you've already listed. Another one is that 2 search strings are easy to compare (so e.g. we can know if the search string changed).

I don't see how this change prevents that. You will still have a string to use after calling the user-provided serializer. Just include a line in the doc stating that (de-)serialization needs to be deterministic to meet RR4's expectations.

It sounds like what we really need is a wiki/doc page that explains the issue that we can just link to w/out the need to explain/comment every time. Don't we already have one?

@pshrmn did post a suggested code example to implement search (de-)serialization in one's own code. The problem is it didn't work when I tried. If it had, I might not still be here. (I still think adding the parse/stringify hooks to history is the right thing to do, but if my code was working right now, I'd probably be focused on the million other problems I need to deal with instead of this one.)

And trying to account for query parse/stringify on the entire history API leads to a bunch of code which most people will never need.

I don't think that's right. The implementation proposed here merely adds two hooks. If they're not used, history need do nothing different. If they are used, history merely need call them at the right time. history does not need to add any opinions, nor add any code but the few more lines to store and invoke the two new options.

Only one breaking change, and that was out of convenience not necessity.

I don't see how this change necessitates breaking anything. It merely adds two new options that are not currently used by anyone to my knowledge.

PS - I'm happy to at least help with testing - and if I feel competent to do so by the end, documenting the solution that is merged. @pshrmn - Comment if/when you feel your fork is ready to be tried by others.

[pshrmn]

It isn't that contributors have thought that automatic parsing isn't convenient. It is just a matter of whether history should be concerned with that or just with navigation.

In v2/3, queries were sort of hacked together by creating an object that intercepts push/replace calls, modified the location object to create a search string, and then passed the new location object to the "real" history. That is what the qhistory package I wrote does as well, and it isn't very satisfying.

I guess that the debate should be centered around what a location object should be. Basically it comes down to finding the right balance of simplicity and convenience. Should a location object be a "pure" representation of the URI, similar to the native Location or should it be more functional by converting the search into something more useful?

We are already going down the road of a non "pure" representation because pathnames are automatically decoded, so it isn't too out there to make the change, although this would be more significant.

While I think that a string is generally good enough, my previous were largely made while thinking about this from a dual search/query layout. Managing the same state between the two was not my idea of a good time. However, I think that making search a clearinghouse for whatever you want can work well. The default behavior should be to expect a string, that way we do not have to provide any built-in query parsing.

// default
search: '?one=two&red=blue'
// with qs
search: { one: 'two', red: 'blue' }
// with URLSearchParams in the future?
search: URLSearchParams{ one: 'two', red: 'blue' }
// for people who like to confuse others
search: function() { return 'one=true&red=blue' }

@odigity You can give the current state of my branch a go if you'd like. You'll have to clone the repo, checkout the correct branch, and npm link it in order to use it, so it isn't the simplest process. If you have any comments on it, leave them in one of the commits over there. I'd rather not clutter this issue up with discussion of code that won't necessarily be implemented.

[mjackson]

the debate should be centered around what a location object should be

I've been trying to keep our location objects as similar to window.location as we can. That way there are less docs to write.

And trying to account for query parse/stringify on the entire history API leads to a bunch of code which most people will never need.

I don't think that's right. The implementation proposed here merely adds two hooks. If they're not used, history need do nothing different. If they are used, history merely need call them at the right time. history does not need to add any opinions, nor add any code but the few more lines to store and invoke the two new options.

You'd be surprised, @odigity. We removed a LOT of code complexity in v4. It's not like we haven't been down this road. We were literally just there. Our API has quite a few methods that manipulate the current location.

Anyway, I really don't want to seem dismissive here but I've done a lot of thinking about this issue and I just don't think it's worth implementing, especially when it's so easy for people to do themselves and there are so many different ways to do it, so I'm going to close. Thanks for the discussion, everyone :)