Update createBrowserHistory.js

[pwmckenna]

add support for forceRefresh

addresses issue: #95

[taion]

This actually isn't going to work if someone just calls createBrowserHistory(). Check how this is done on e.g. createHashHistory - we use a default value for options, and use destructuring to pull out the param:

https://github.com/rackt/history/blob/v1.12.5/modules/createHashHistory.js#L39
https://github.com/rackt/history/blob/v1.12.5/modules/createHashHistory.js#L45

[pwmckenna]

@taion thanks for the hand holding...you guys clearly care about a gentle onboarding. much appreciated.

[taion]

Indent here is inconsistent with the rest... should be 2 spaces rather than 3.

[taion]

We should also document this somewhere.

[jlongster]

I just tried this out, and there's still a bug. When the location changes, it still fires all listeners immediately. That means that I see the page "glitch" immediately for a second, until the window reload occurs and actually navigates to the new URL.

We need a way to tell history not to actually internally update its location if we are doing this. One way to do this is in transitionTo in createHistory.js, allow finishTransition to return a boolean value indicating if the location should actually be updated. It could return false to forcefully stop it, just like an onClick handler. The new code would look something like this:

      if (ok) {
        if(finishTransition(nextLocation) !== false) {
          updateLocation(nextLocation);
        }
      } else if (location && nextLocation.action === _Actions.POP) {
        ...
      }

I just tried this and it works.

[pwmckenna]

@jlongster would you mind describing how you're manually testing. sorry if that sounds overly rudimentary.

[jlongster]

@pwmckenna I just modified the react-router code in my project manually. If you're talking about which projects, I'm testing it on my blog: https://github.com/jlongster/blog

But it's probably not worth setting it up. Just try it on a simple project, and maybe delay the window.location.href = ... line by a second or so, and you'll see it.

[taion]

@jlongster IMO that's a separate bug though? Would the same issue not arise if the History API were actually unavailable?

[jlongster]

@taion That's true, yes it would have the same issue. Seems like we could still fix it in here though, unless you really want a separate PR?

[taion]

I think it might be slightly nicer since it's a separate atomic thing that's orthogonal to this.

[jlongster]

Ok done: #103. If accepted this should add return false after doing window.location.href = ...

[taion]

I don't understand the test failure so I'm just blindly re-running the PR build. This looks good to me.

[taion]

LGTM. Not sure what our policy for merging stuff is so I'll let someone else hit the green button 😄

[taion]

@jlongster Sorry what I meant was that the return false part or whatever should also go in #103, since that's an orthogonal fix that also covers the existing case where HTML5 history is not supported. Just makes the diffs cleaner to read.

[mjackson]

Looks great. Thanks, @pwmckenna!