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

Session history tracking #843

Closed
wants to merge 5 commits into from

Conversation

agundermann
Copy link
Contributor

(Work in progress)

This PR makes the router more aware of the current session history by managing a counter and an id that's stored inside history.state. Before every push, the counter is incremented, and on every location change, it is reloaded from the active state (or reset). The (random) id is used to distinguish between two "sessions" within the same browser tab.

Since only HistoryLocation supports state, I also added utils/addLocationState.js which can be used to wrap other locations to attach state to the query (for local development, or environments without url bar). Not sure if it's worth having though.

The history state is currently exposed via state.history:

  • state.history.current: index of currently active session entry (1-based)
  • state.history.id: random string id for current "session"; each id has its own series of entries
  • state.history.key: key for caching based on active entry in browser tab (concatenation of the two above, for convenience)

Use cases:

  • more reliable goBack() check (already implemented), perhaps also goForward() and go(n)
  • scroll position can be saved by history entry, not just by path (already implemented)
  • persisting arbitrary data based on browser session entry (e.g. form data)
  • basis for more advanced navigation (Advanced navigation patterns #767), such as going back or forward to when a route was last visited
  • animations based on navigation direction (back vs forward), or even pages that were traversed

Seems to be related to #828.

@gaearon
Copy link
Contributor

gaearon commented Feb 16, 2015

That's some great stuff. Does this supersede #828?

cc @insin

@insin
Copy link
Contributor

insin commented Feb 16, 2015

@gaearon The extra argument both PRs add to location changing methods definitely seems to be the right place for all things related to the URL, but not contained within the URL.

However, the use case which triggered #828 was getting ephemeral data into and out of a willTransitionTo hook when using a route handler like a request handler, which this PR doesn't address. It also applies to Hash and Static locations, as the extra data is just passed straight through.

Seems like that extra argument will need a bit of design work if all related-to-URL-but-not-in-URL data have to go through it.

@mjackson
Copy link
Member

@taurose This is excellent stuff! Thank you! :D

A few questions, just to make sure I understand what's going on here:

  • goBack is more reliable with this approach because we are loading the counter from location state instead of just keeping it around in memory, right?
  • I think the new StaticLocation can also support this, right? Maybe even TestLocation...
  • How would you feel about augmenting the History module to be smarter on platforms that support HTML5 history instead of keeping both History.length and location.getCurrentState().current ? It seems like those two counters are more or less trying to do the same thing. Eventually we could maybe leverage window.sessionStorage for platforms that don't support HTML5 history, or even to increase the capacity of HTML5 history state storage.
  • Can we possibly use this to get a more reliable action type onhashchange? Right now we're just guessing the action was LocationActions.POP when the hash changes but it would be great to be more certain here.

Sorry if that's a bit of a brain dump! Excited about the direction this is headed though :)

Seems like that extra argument will need a bit of design work if all related-to-URL-but-not-in-URL data have to go through it.

@insin What do you mean? Sorry, just trying to understand. :)

@agundermann
Copy link
Contributor Author

  • goBack is more reliable with this approach because we are loading the counter from location state instead of just keeping it around in memory, right?

That's just one reason. The current implementation doesn't take pop events into account at all. So if you click a link on a freshly loaded page, click browser back and then use .goBack(), you'll be taken off the page.

  • I think the new StaticLocation can also support this, right? Maybe even TestLocation...

I don't see any point in using this for StaticLocation. The purpose is to keep track/be aware of client-side browser session history. With StaticLocation rendering server-side, I don't think we have any way to access this information, unless it was embedded into the URL.
It should be possible with TestLocation, or any other in-memory location. I wouldn't be surprised if it already worked with the wrapper utility.

  • How would you feel about augmenting the History module to be smarter on platforms that support HTML5 history instead of keeping both History.length and location.getCurrentState().current ? It seems like those two counters are more or less trying to do the same thing.

I think that would make sense. I'll see what I can do, though I don't like the idea of the data being stored and modified in a global module (like History.js currently is). So I'd have to add something like location.setHistory.

  • Eventually we could maybe leverage window.sessionStorage (...) to increase the capacity of HTML5 history state storage.

Well, the PR gives you state.history.key, which you can use to persist your data yourself i.e. sessionStorage.set(state.history.key, data). It doesn't allow you to write anything into the history.state object yourself, that's all managed by the router. However, we could build #828 (payload object) on top of that, either by using the location state object as well, or by utilizing state.history.key (persistence for browser reload could then be added with sessionStorage).

  • Eventually we could maybe leverage window.sessionStorage for platforms that don't support HTML5 history

I don't think it's possible at all to make this work with anything but HistoryLocation without embedding state into the URL. When the user clicks back or forward, we have no way of telling how many steps he went forward or backward. I don't see how sessionStorage would help there.

  • Can we possibly use this to get a more reliable action type onhashchange? Right now we're just guessing the action was LocationActions.POP when the hash changes but it would be great to be more certain here.

First of all I should say that this comment about LocationActions.POP is off:

Indicates the most recent entry should be removed from the history stack.

To quote MDN:

A popstate event is dispatched to the window every time the active history entry changes between two history entries for the same document.

In other words, POP simply means we've gone to a page we've been to before within our app. Even with pushState API by itself you don't get any information about whether the user pressed forward, back, or how many pages have been traversed, hence this PR.

That said, I don't think the current workaround is that bad. The session tracking could make it a bit more robust I guess, by only using POP if the current state contains state which we set before. One scenario where the state would not exist is when changing the URL manually. However, what would we do instead in these cases? I'm not sure if it's safe to assume it was a PUSH..

But, as I said, I don't think we can make this PR work reliably with HashLocation without messing up URLs..

@mjackson
Copy link
Member

@taurose Thank you for your detailed response :) I think I'm starting to understand this better.

I don't think we can make this PR work reliably with HashLocation without messing up URLs

I see. Looking over the API of history.js it looks like they were able to leverage sessionStorage with a single query parameter (I'm guessing the session id). It would be great if we could do the same thing when people use state with HashLocation.

I'm also starting to think that the Location interface should change to more closely match the History interface. So every location would have pushState and replaceState instead of push and replace. It would be great if we could tell people they can use pushState regardless of which location they are using.

@mjackson
Copy link
Member

@taurose I don't know if it helps, but I've been playing around with a new History API in the history-api branch, inspired by your work here.

What do you think of the API in 7b9ad81 ? I was trying to enable some of the stuff you mentioned in #767 while working on it.

@agundermann
Copy link
Contributor Author

I didn't add state support directly to HashLocation because of the broken URLs; instead, I wanted it be opt-in. Do you intend to make it so that state is always used when using HashLocation?

As far as I can tell, HistoryJS manages with one param because they don't have to provide session history information (history.current). Theoretically, we could also use sessionStorage + random state ids for that. But if we have a reliable way to provide history.key, then HistoryLocation could utilize sessionStorage as well, keeping history state to a minimum and storing data in sessionStorage instead (like you suggested). That's why I added the state to the query in the first place.

So I don't think making HashLocation add query params and handling sessionStorage by itself would add much value, but only add another layer of abstraction.

Your History.js looks good so far. I don't think History.length can work like that though. What I had in mind for that was to manage .length for each sessionId (and store in sessionStorage). We also need a sessionId (or History.key) to be able to uniquely identify a specific browser history entry (there can be more than one "session" of a single app within a browser tab, i.e. sessionStorage keys would conflict when just using .current).

If we are to support Locations without state, what do you think about having a TrackedHistory and a normal History? They would provide the more complete History API and delegate the pushState API to the given Location. The tracked one would support .current, .canGo(n) etc, and could also take care of sessionStorage (abstracting the state object - similar to the abstraction you suggested for HashLocation, but at another level), while the normal one returns reasonable defaults, or throws/ignores.

As far as Location and Router APIs are concerned I'd see two options:

  1. Like in this PR: HashLocation etc. don't support state by default, but can be wrapped with something like embedStateIntoQuery, and createRouter.js chooses History type depending one state support of given location
  2. HashLocation etc. support state by default by embedding it into the query. People can decide whether to use TrackedHistory (and thus add query params) via Router.create or Router.run.

@agundermann
Copy link
Contributor Author

I just noticed that changing the hash (manually or via <a href="#hash">) will cause a popstate event with state null. Previously, the routers history data would be reset (fresh id, current = 1) to be on the safe side. I changed it to keep the data and replace the state of the location object with the last known history data instead. This way, changing the hash will not change the history key.

I'm a bit worried that there could be other cases where popstate state ends up being null, though..

Edit: this breaks canGo(n) :(

mjackson added a commit that referenced this pull request Feb 27, 2015
Please note: this commit is still a work in progress!

All history objects subclass History and support 2 main methods:

- pushState(state, path)
- replaceState(state, path)

This API more closely matches the HTML5 history API, with the notable
omission of the title argument which is currently ignored in all major
browsers. It provides the user with the ability to store state specific
to the current invocation of the current URL without storing that data
in the URL itself. However, history objects that do not use the HTML5
history API (HashHistory and RefreshHistory) store their state ID in
the query string.

This should help with #767 and #828.

This work was inspired by work done by @taurose in #843 and @insin
in #828.
@mjackson
Copy link
Member

@taurose I just pushed some work related to this in 212b9d5.

I'm thinking at this point we will probably benefit from having every location (renamed to "history" in that commit) support state other that what they can keep in the URL, so all of them have pushState(state, path) and replaceState(state, path) methods. Also, when the location changes history objects emit a Location object that wraps path and state inside it, so we don't need to pass around two arguments all over our API.

Would you please take a look at that commit (it's still a work in progress) and let me know if you think you can build sessions on top of it? Thanks again for all your work on this. I'm not trying to supersede it :) Just trying to understand the problem better for myself and perhaps approach it from a slightly different angle. I'm totally open to any feedback/suggestions.

@agundermann
Copy link
Contributor Author

I've given up on history.current and canGo(n) for browser-based histories. There are just too many browser inconsistencies to make it work reliably, especially related to hash navigation. I still think it's worth supporting these methods for other, less restrictive environments though.

Your commit looks great thus far. However, as much as I like sticking to the HTML5 history API, I'm still not fully convinced it's the best approach. How would you handle setting state after a transition happened? For example saving some view state after a popstate?

Edit: Also, don't you think there's a value in allowing people using HashLocation or RefreshLocation to opt out of query modifications (and have state stored by path, or be ignored)?

@agundermann
Copy link
Contributor Author

@mjackson Any thoughts on this? I'm thinking in order to support setting state after a popstate, we'd have to extend the API somehow. However, I see no way in making this work with HTML5History without sessionStorage.

At this point, it feels like we could just have the router manage sessionStorage and state, while the History objects provide keys. For example, we could extend the Location object to provide .key and optionally .current.

@mjackson
Copy link
Member

@taurose I agree with you that this is the right approach. Let's do this work on top of #1158 and get rid of canGo(n) in that branch. I might have time to do it today/tomorrow. Otherwise, would you like to do it (also to make sure I don't screw things up too badly? :P )

@agundermann
Copy link
Contributor Author

Sure, I can work on it later today or tomorrow if you haven't done it already.

@mjackson
Copy link
Member

@taurose I'm doing some work around this today, but I don't want to step on your toes. Your insight is super valuable. Want to pair on it somehow?

@agundermann
Copy link
Contributor Author

@mjackson No worries; I hadn't gotten around to actually start writing. But I think I would

  • push the current .current and .length properties from AbstractHistory down to History
  • expose this kind of history information with a new, optional getEntry() and add it to the Location object in getLocation(). getEntry() must either return an object with just an id (just enough for sessionStorage), or an object with id, current, and length
  • change AbstractHistory.canGo() to make use of getEntry() instead
  • Implementations
    • BrowserHistory: returns unique, randomly generated id via getEntry() which is stored in the browser's state object
    • HashHistory&RefreshHistory: do not support getEntry() by default, but perhaps using a wrapper or subclass which stores information in URL
    • History: exposes all of current, length, and id (id could be same as current) via getEntry(). Manages those properties just like it does now.

@ryanflorence
Copy link
Member

can we close this?

@taion
Copy link
Contributor

taion commented Dec 1, 2015

I'm a little bit swamped right now, but I think we need something like this for remix-run/history#157 and to handle remix-run/history#104 cleanly.

@taion
Copy link
Contributor

taion commented Dec 1, 2015

I meant that in the sense that I haven't had a chance to read through this PR thread - but I will do so when I get a moment.

@lock lock bot locked as resolved and limited conversation to collaborators Jan 20, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants