State management in hash history

[mjackson]

We should eliminate the query string when users aren't actually using state (i.e. state is null) with hash history. This would make ugly hash URLs slightly less ugly.

[taion]

This is tricky - I like the idea of having cleaner URLs, but it seems tricky then to be able to generically support updating location state without causing a full re-render of the page.

Among other things, without allowing that, I'm not aware of a good way to build support for browser-like scrolling that doesn't break horribly on e.g. Firefox - the sequencing of events on a POP transition means that you have to store the scroll position as you go, because you don't get a final "I'm about to leave the page" callback (and even then you'd still need to store the location somewhere, because the sequencing of the browser scroll restoration there does not give us what we want for free, the way we get it on Chrome).

Maybe a better approach would be to implicitly use e.g. a hash of the current location as the state key when not using a query key, and make that the default, then warn when attempting to use state. It opens up a few edge cases, but I think it's a worthwhile trade-off.

[mjackson]

it seems tricky then to be able to generically support updating location state without causing a full re-render of the page

Agreed, this case becomes a lot more tricky. Assuming we actually have a way to do this (i.e. bring setState back from deprecation-land) maybe we could have a way to opt-in to always having query keys for the people who know they'll need them. So, e.g. if I know I'll need to use setState I could just say something like:

const history = createHashHistory({ alwaysEnableState: true })

I'm just trying to provide a better default for people who are using hash history but not using state. It's pretty low priority, because hash history is a hack anyway, but a lot of people still seem to be using it. And we still use it as the default in the router.

[taion]

If you're calling it a hack, how about we just use window.name for the location key instead?

[taion]

Oops, never mind, that doesn't work that way.

[taion]

I'd leave setState deprecated - I think the new replace ought to supersede most use cases of setState; the only thing it'd be missing would be not re-running route matching in the router, but that's not really a first-order concern for me. With the new API, you'd just do:

history.replace({...location, state: {...location.state, ...nextState}})

or whatever and get more control over what exactly is happening.

Here's a somewhat complicated proposal that I think would mostly "just work":

1. By default, don't use a query key
2. If not using a query key, invariant if supplying state to push and replace
3. If not using a query key, still provide location.key, but hash it on the location path

Why (2) and (3)?

(2) is to prevent users without query keys from explicitly using state with most normal patterns - it'd be horribly unsafe to use state as the in-browser equivalent of POST data if we don't actually have location keys, and typically that is done via these APIs.

(3) is to let scroll behavior work - I don't want to fire listeners on persisting scroll position anyway, because that would lead to incredible re-rendering jank. Furthermore, for something like that, it's acceptable to bind the state to the path (in fact this is what the Gmail site does - if you navigate around, you'll see that scroll location for two history entries with the same path is tracked the same). This is a much less powerful API than per-entry state management, but it's lower stakes and can be more broadly applicable.

[agundermann]

Sounds like a good solution. The only troubling scenario I can think of is when using location.key (eg for storage) and expecting it to be different for the same URL. But I guess we can live with that.

[taion]

Note https://github.com/mjackson/history/commit/f12d632691ebef652d0cf5dd67a033e87746fc4a#commitcomment-16590812 – I don't think we can get away from having to generate a key from a hash of the URL if we want to maintain reasonable semantics around location.key.

[ryanflorence]

The only troubling scenario

Can we keep location.key for browserHistory?

[taion]

This doesn't change it for browserHistory. I think hash-of-the-location-based keys are going to be "good enough" for hash history.

I'm thinking about this in the context of scroll behavior – we need to always have access to some sort of key to be able to save down the scroll behavior in session storage.

[taion]

Let's keep this open to track the remaining issues with having a location key.

[donpark]

Why not defer creating location state key until it's actually needed instead of levying ugly tax on everyone?

Many of us history module only thorough react-router and don't use location state so I think this ?_k=asdf should be made optional and on-demand only.

[mjackson]

That's exactly what this issue is about, Don. On Fri, Mar 25, 2016 at 2:23 PM Don Park notifications@github.com wrote:

Why not defer creating location state key until it's actually needed instead of levying ugly tax on everyone?

Many of us history module only thorough react-router and don't use location state so I think this ?_k=asdf should be made optional and on-demand only.

— You are receiving this because you modified the open/close state. Reply to this email directly or view it on GitHub https://github.com/mjackson/history/issues/163#issuecomment-201512669

[donpark]

Yeah. I'm just frustrated by lack of movement on this when I see this change as not a mere enhancement but essential change.

I would just use the full path+fragment as the location state key. So key for route /foo/bar using hashHistory will be /#/foo/bar. Apps that need different states for same route will need to extend the route path which is easy to do and IMO should be surfaced instead of handled magically below deck.

[taion]

I like @donpark's idea of just using the contents of the hash as the default location.key. Of course, we'd want to add in ?_k when the user explicitly calls push or replace with state, and do some appropriate deduping (maybe just prefix the "implicit" keys with # or something).

This would be enough to deliver scroll behavior experience that matches e.g. the Gmail webapp for apps using hash history, which is good enough for me.

[mjackson]

@donpark

I'm just frustrated by lack of movement on this

As far as I'm concerned this issue is resolved in master. You can try it out right now in the 3.0 beta: npm install history@next.

I would just use the full path+fragment as the location state key.

You don't need my permission to do this. In fact, I'd rather history give you location.key === undefined than do this for you so that you have a way to tell when you have an authentically unique key and when you don't.

const key = location.key || window.location.href

This way you can at least opt in to potentially overwriting some previous state with your eyes wide open, because you chose they key. We already told you that the location doesn't have a unique key.

@taion I'm not sure what you mean by "reasonable semantics around location.key". There are already reasonable, reliable semantics around location.key as it currently exists in master.

We used to (in 2.x) say:

location objects always have a unique key property, no matter what

This means that when we don't actually have a key on the initial getCurrentLocation, we need to use replaceState (or trigger a hashchange) in order to get one. This causes problems like the one in #220 and feels too aggressive for a simple "get" operation anyway.

In 3.x we say:

location objects may have a unique key property. If they don't, you can try generating your own key using the URL.

At least now users will know if they have an authentic location.key or not, so they can decide to use the URL, which is potentially risky because they can visit that same URL again at a different place in the history stack. More information for users is better than less.

At this point I'm assuming this issue is still open because of confusion around how the semantics have changed. But as far as I'm concerned we can close.

[donpark]

Fine. I'll just fork my way through the problem since we don't agree.

[mjackson]

@donpark Good luck!

[taion]

I still have open concerns here. Need to find some time to write them up.

[taion]

@donpark I think the current implementation on master actually does more or less what you want. However, it doesn't do what I want.

For location.key, consider why we need it (and the predecessor setState) at all. For managing scroll behavior, assume for the sake of argument that we need to persist the scroll location on our side. Alternatively, consider something like an infinite scrolling list (like on the Instagram web front end) where you need to keep track of how many items have been loaded so far, so the user gets a consistent experience upon hitting "back".

Why do we need something like setState in the first place? The problem is that replace is overkill. It would cause the router to re-match and re-render everything, which would cause unacceptable jank on scrolling. You might argue that this isn't strictly necessary on the router side, which is true, but we still don't want to create a new history entry every time the user scrolls, because that would fill up session storage very quickly, and also not play nicely with the Redux integrations.

So the requirement really is that these integrations like scroll behavior management need to be able to persist session storage in a way that's attached to the history entry. While technically all of this can be handled outside of history, it'd be a mess – basically it'd require making a separate independent wrapper around session storage, which is just really unnecessary.

Thinking about this a bit more, maybe the best approach is to just bring back setState. If you think through what's required to build a reasonable implementation of either of the cases I've mentioned above, we can't get around having some way to persist state without replacing the entire history entry, and this handling really belongs in history rather than in downstream packages, because it's a common thing, and the point here is to offer a consistent API with predictable semantics.

[taion]

I am going to keep this issue open because the current semantics on 3.x are really not good for building reasonable extensible abstractions.

It shouldn't be necessary to special-case hash history when building a wrapper for something like scroll behavior management.

It doesn't make sense to do something like const key = location.key || window.location.href because that requires making particular assumptions about the specific form of the history used – what if it's memory history or something?

It's not a worthwhile tradeoff to make it significantly more difficult to build critically useful things like scroll management just to keep the code here a bit cleaner. Need to keep track of why we have things like location.key in the first place, and the actual downstream use cases.

[taion]

Updating the issue title to be more generic. The remaining problems are the following:

1. When you PUSH or REPLACE a new location on hash history, location.key actually is set to something random; the key only goes away when you POP to a location without state – so contra https://github.com/mjackson/history/issues/163#issuecomment-201560721, hash history locations actually will have location.key as something defined, but it won't be anything that can be recovered
2. You generally want location.key for a setState-like operation, with the same semantics as history's state management, so maybe the thing to do is to give that method a more descriptive name, something like setLocationStateInPlace – a better way to signpost that most of the time, people should just use replace
3. For hash history, we should rip out the query key management handling altogether, and just make the key be some deterministic function of the current location – if users want the kind of distinct state management, they can do query: { _k: createKey() } themselves; as long as we document this, I think it'd be pretty close to the location state management semantics that should be expected from hash history anyway (and do things like e.g. clearing the state when pushing back to the same location)

[mjackson]

First of all, please excuse a small rant: I'm soooooo tired of addressing the problems of hash history. Hash history is an old hack and is only required in IE <= 9 which Microsoft themselves don't even support anymore. Let's please keep that in mind when working on this library. I'm not saying we don't support hash history; we do. But at this point the vast majority of the discussions that happen on this repo have to do with hash history which really isn't that interesting. Let's move stuff forward and encourage our users to do the same. It's a difference of focus, that's all. I'm doing my best to focus on the future, not the past.

Ok, enough ranting.

hash history locations actually will have location.key as something defined, but it won't be anything that can be recovered

That sounds like a bug. I'll push a fix. We shouldn't give people a location.key if we don't intend to give it back to them later.

For hash history, we should rip out the query key management handling altogether, and just make the key be some deterministic function of the current location

This statement demonstrates a fundamental misunderstanding of what location.key is. I think @donpark may be confused about this as well.

location.key would be totally redundant if it were a simple hash of the URL. Users could do that themselves. There would be no purpose in us doing it for them. I'm going to try and explain this one more time, and then I'm going to close this issue for the 3rd time.

Forget about building your scroll management thing, and the router, and everything else for just a minute, and imagine the following scenario:

1. POP /home - the user loads the home page
2. PUSH /about - the user clicks on a link
3. POP /home - the user clicks the back button

In this scenario there are 2 entries in the history stack, and we're at the first one (index 0). Contrast that with the following:

1. POP /home - the user loads the home page
2. PUSH /about - the user clicks on a link
3. PUSH /home - the user clicks on a link back to the home page

In this scenario, there are 3 entries in the history stack, and we're at the 3rd one (index 2). The URLs in this scenario are identical to those in the first scenario, but the semantics are different.

Now, if we calculate location.key from a simple hash of the URL then these scenarios would look like this:

1. POP /home - location.key = hash(/home)
2. PUSH /about - location.key = hash(/about)
3. POP /home - location.key = hash(/home)

In this scenario, a simple hash of the URL would be ok because POP /home is actually taking us back to the same index in the history stack. However, if we do the same in the second scenario:

1. POP /home - location.key = hash(/home)
2. PUSH /about - location.key = hash(/about)
3. PUSH /home - location.key = hash(/home) <-- LIES/ERRORS/SADNESS/DEATH

In this scenario the 3rd step actually takes us to a different location than the location in step 1, though the URLs are identical. This is why we can't generate a key from a hash of the URL. This is also why we can't reliably give you a key without putting it in the URL query string, because that's the only source of truth we have to go on when using hash history.

Internally we use location.key to persist location.state to sessionStorage. That's good enough for our use cases internally. If you're trying to build something on top of hash history, and you need a key, then your guess is better than ours is on what would make a good key for you. We are only going to generate one, and put it in the URL, if you give us some state to keep track of. Otherwise, that's not our job.

[taion]

That's probably not the main use case of hash history at this point. Most users of hash history are probably using it to avoid having to configure a server – it's quite useful to not have to configure a server to set up many smaller web applications, especially ones that aren't meant as external-facing user applications.

Moreover, as we've already discussed on https://github.com/mjackson/history/issues/231#issuecomment-194417099, we are clearly not seeing eye-to-eye at this point as to the proper integration point between history and the router.

If you're not especially interested in maintaining something that's a useful baseline abstraction for building a router and e.g. stripping out things like createHref so that some higher-level abstraction needs to unify "hash history + hash hrefs", or "basename + hrefs with basename", that's fine, but all that means is that we'd need to build something separate to wrap those concepts, or else swap out the dependency on history entirely.

That's your call, but I'd rather this library be more useful rather than less useful.

[donpark]

I chose to fork because @mjackson thinks this is a low-priority technical issue while it's a major UX issue to me. No amount of conversation can bridge the difference.

[taion]

That actually makes no sense. Just use a custom history and do createHashHistory({ queryKey: false }), if you're using v2.

[donpark]

with useRouterHistory wrap you mean. I may do just that now and revisit when application state gets more complex.

[taion]

Yes, exactly. History v3 is not a drop-in replacement for v2 anyway so this discussion isn't going to affect anything in the near term for current router users. Just use the custom history path for now. Location state is often of somewhat limited use anyway, outside of things like scroll management.

[donpark]

Yeah. Too bad one has to read source code to figure this stuff out. Ideal API for this stuff to me is to just set location hash as needed when I need to associate some state like when user scrolls. Having full control over URL is I think what most developers want. Anyway, thx for the help @talon.

[taion]

This is in the docs: https://github.com/reactjs/react-router/search?utf8=%E2%9C%93&q=queryKey

[donpark]

Must have missed that. I found useRouterHistory when I ran across queryKey warning in the source code and createXXXHistory import chain.

[mjackson]

@donpark you haven't addressed my comments and you refuse to acknowledge the fact that your "UX problem" is actually already fixed in v3, which you can install and use right now if you wanted. Please, fork away. Hopefully that means you won't waste any more of our time here.

@taion I'm done discussing on this thread. If you think the library has some defect you just can't overcome, let's discuss in a new issue. IMO you're just not being creative enough. On Sat, Mar 26, 2016 at 2:18 PM Don Park notifications@github.com wrote:

Must have missed that. I found useRouterHistory when I ran across queryKey warning in the source code and createXXXHistory import chain.

— You are receiving this because you were mentioned. Reply to this email directly or view it on GitHub https://github.com/mjackson/history/issues/163#issuecomment-201930627

[donpark]

@mjackson no I didn't bother looking at v3 beta because I was and still am too busy. your abrasive tone didn't help either. Be at peace knowing that I won't be wasting any more of your time. Good day to ya.

[taion]

@mjackson

It's not a matter of creativity or lack thereof. Trivially dependencies can accomplish whatever they want through some combination of hackery, code duplication, and calling into private APIs.

The consideration in question is one of making it as easy as possible for libraries extending other libraries to do what their authors need to do. It's not a question of whether it's possible – it's a question of what semantics best allow people to do what they want to do. Abstraction for its own sake is not a good goal.

My point here is that the lessons we've learned so far in building out scroll-behavior should inform how we choose to design interaction going forward; just as the lessons in building data integrations like async-props and react-router-relay shaped our development of the React Router API – and those lessons point against the location.key semantics that you are proposing.

[mjackson]

@donpark

no I didn't bother looking at v3 beta because I was and still am too busy

You have time to come here and complain, but you don't have time to look at the fix?