Update Histories

[agundermann]

See #1363 for previous discussion.

[mjackson]

Awesome. I'm really looking forward to getting this merged, @taurose. Are you planning on merging as-is? Or is there more work you'd like to put in here?

Not sure if you saw my comment on #1363...

[agundermann]

Working on it :)

Some thoughts:

• we can't get aborting POP events right as I explained here. I think the best option would be to make aborting those a no-op, i.e. the URL would be out of sync, but the browser history is left untouched
• setting unload events disables Firefox Caching, which would actually fix Firefox rendering wrong URL #837, but might be a reason to let people opt out if they want to use the cache

[agundermann]

@mjackson Added a new commit for onBeforeChange. Only a single listener is supported right now (not sure if it's worth adding more). Callback signature is (location, navigate), where navigate is a callback that performs the actual navigation; it can be called asynchronously or not at all. For DOM unload, location and navigate are not set, and the method is expected to return a string for the dialog message.

Also, I think the History classes would be a bit cleaner if all the listener setup happened in setup, and unbinding in an extra teardown method. Any objections?

[mjackson]

Any objections?

Not at all :) When we made the switch from providing a singleton to requiring users to instantiate a new History object, the setup/teardown model changed a bit I think.

[mjackson]

we can't get aborting POP events right

I completely agree. That's why I want the "before change" listener, so users can choose to cancel navigation if they want to. Hooking into "before unload" is just one time we need to trigger "before change". We also need to trigger "before change" every time we're about to push/replace programmatically in case the user wants to abort that transition. Are we doing that yet?

[mjackson]

I also think this work will be an initial step in fixing #1356. We'll need to split up running transition hooks into two stages: "will transition" and "did transition". All redirects need to happen in the "will transition" stage, so we don't call hooks that users intend to run after the transition has been made twice.

[agundermann]

Not sure about Redirects, but it sounds like most of what you mentioned covered.

history.onBeforeChange((location, navigate) => {
  if (!location) {
    // it's the dom unload event, so we can only return a message sync 
    // (I guess this part could be more expressive, but I kept it simple for now)
    return "Do you really want to leave?";
  }

  // note that for PUSH&REPLACE, location.state does not contain entry information 
  // from subclasses (like key)
  doSomething(location, (err) => {
    if (err) {
      handleError(err);
      // abort by not navigating
      return;
    }

    // this will perform the actual navigation
    // URL change and saving state will only happen at this point
    // except for Browser&HashHistory POP (URL is changed before event)
    var navigationResult = navigate(); 

    if (!navigationResult) {
      // navigation was superceded
    }
  });
});

Writing this down, I feel like we also need history.isPending(location), don't we?

Also, do you think a separate beforeChange handler is the right approach? We could probably do the same with a single change handler.

[mjackson]

@taurose Can we get the tests passing here? I'd like to merge ASAP! :)

[agundermann]

@mjackson They should pass.. Travis failed because of some license error, I have no idea what that's about 😄

[mjackson]

Thanks @taurose. I'm looking through the code right now, going to try to get it merged soon. I'm having a bit of trouble following the logic in some places, but I think it's probably more due to my use of subclasses than your code :) I may try to simplify it a little.

[agundermann]

@mjackson Sure, go ahead :) Let me know if you have any questions. I'd also write some tests and documentation, but I wasn't sure if the new design and API is what you wanted to see. Also, I agree that using class hierarchies feels a bit off at this point.

[mjackson]

I agree that using class hierarchies feels a bit off at this point

I wonder if we could clean things up using components instead of classes. A few things about these classes makes me think we could, namely:

• variable constructor arguments could just be different propTypes
• setup/teardown could just be willMount/willUnmount
• auto-binding (using createClass)

There may be a few other benefits as well. Should we give it a shot? We had considered history components briefly in the next branch, but decided against them ultimately because we need to be able to respond immediately to redirect. But that problem is solved using the transition.to API. I think it could work...

[agundermann]

@mjackson I don't think using components will help much. There's nothing React components can do that can't be done with plain JS, so I think we'd just limit ourselves. Ideally, it should be easy to create wrapper components though.

I've done some testing for canGo(n) and created a branch here. It includes new examples that display current, length, key and the internal data structure.

However, I've once again come to the conclusion that we can't provide canGo(n). Apart from browser inconsistencies with hash links and refreshing, we can't reliably track .length. The reason is that when our app is left, we have no way of knowing how it was left. We do get the unload event and know .current, but we don't know what happens to our session. For example, leaving with browser back or forward would leave the session as is, but clicking a bookmark would delete all following entries (like a PUSH). In that case, going back to our app would cause .length to be too high and canGoForward to give false positives.

We might be able to provide .current and thus canGoBack(n), but I'm not sure how reliable it would end up being.

[mjackson]

Thanks for investigating this @taurose. I apologize for the silence. I was in Paris last week and things got kind of busy :)

I almost have a commit ready to push onto this branch. I agree w/ you that components won't help much, so I'm sticking with the classes. I have, however, tried to reduce the amount of indirection in the code. There are just a few questions I have about your work before I push.

1. In _doPushState, you do a replace instead of a push if the path is the same. Why is that necessary?
2. Since we're using sessionStorage for both DOM histories now, can we get rid of location.state.key? It seems like we're the only ones who actually need it, because the user can put whatever they want in location.state now.

As far as canGo, I think I've worked around the router's need for that functionality, for now at least. My commit will introduce a confirmTransition method that histories will use to show a confirmation dialog to the user (DOM histories just use window.confirm). In the router, our main need to know whether or not we could go back was so that we could safely goBack when the user aborted a transition. However, in my commit we avoid having to make that decision entirely by removing transition.abort and only using window.history.back() inside popstate/hashchange event handlers (so we know we have at least one entry in the history). I believe we can move forward without canGo for now, and possibly add it back at some future date, maybe when we don't need to support window.location.hash anymore..

[agundermann]

I have, however, tried to reduce the amount of indirection in the code.

Sounds good 😃

In _doPushState, you do a replace instead of a push if the path is the same. Why is that necessary?

It's for #826. HashHistory is unable to push the current path (location.hash = location.hash doesn't cause navigation or fire hashchange). Since that's also the default browser behavior (clicking the same link multiple items won't create new entries in the history stack, but only cause reloads), it seemed reasonable to me to put that behavior into the base History class. An alternative could have been to put it into the other history classes (BrowserHistory and MemoryHistory).

Since we're using sessionStorage for both DOM histories now, can we get rid of location.state.key? It seems like we're the only ones who actually need it, because the user can put whatever they want in location.state now.

You mean simply not expose it? I'm not sure that's a good idea. I think it best answers the question of "Where am I?". It might be useful as a React key for transitions based on navigation, or for using a storage different from sessionStorage which we provide. That said, I'd actually prefer it as location.key.

DOM histories just use window.confirm

Isn't window.confirm a bit limited? I personally wouldn't want to use it unless I had to (eg. with beforeunload).

only using window.history.back() inside popstate/hashchange event handlers

You mean push? history.back could take you off the app after jumping to the first entry.

[mjackson]

I'd actually prefer it as location.key

Me too. Let's do it.

Isn't window.confirm a bit limited?

What I meant is they use window.confirm by default. You can of course use your own confirm dialog if you'd like.

history.back could take you off the app after jumping to the first entry

Great point. I'll just have to manually revert to the last known location in that case.

@taurose In general I think the histories are complex enough that they actually deserve their own repo and package. I've been working on them quite a bit, and it's really starting to feel like I wish we had something decoupled from the router repo to work with.

Would you be ok if we 1) merged this PR and then 2) created a new repo with your code + any additions I have?

[agundermann]

Would you be ok if we 1) merged this PR and then 2) created a new repo with your code + any additions I have?

Sure. I think it's a good idea.

Can't wait to see your changes. I'm not sure exactly where you're going with removing transition.abort and using window.confirm at this point.

[mjackson]

@taurose I've created a new repo to contain all the history stuff at https://github.com/rackt/history. This should help us separate the responsibilities a little more cleanly. The history package doesn't know/care about routing at all.