Prevent pushing duplicate paths to the history API.

[guidobouman]

Fixes #507

[guidobouman]

Okay, this is a little bigger...

HashHistory emits a warning about replacing instead of pushing to the stack. But this is actually the WHATWG standards behaviour. So should I remove the warning from the HashHistory and homogenise the implementation around BrowserHistory, MemoryHistory & HashHistory?

[timdorr]

You're only comparing the pathname in this case. What about the search/querystring? Since you're doing this in multiple histories, you should export a common function, like locationsAreEqual.

[guidobouman]

Great tip, thanks.

There's already a locationsAreEqual method, which is great. But it also checks for key, something we don't want. Should I add an override to the method?

That would result in the following signature:

locationsAreEqual = (a, b, ignoreKey) => { ... }

Sounds reasonable if you ask me. On the other hand, invoking the function gets a bit vague that way:

locationsAreEqual(location, oldLocation, true)

I could also create a new method locationsExceptKeyAreEqual or locationEqualsDestination. What do you think?

[timdorr]

Hmm, I'm not sure why key is in there at all. If the key is equal, the locations are equal. That's the point of the key value being in there. So, that function would appear to be fundamentally flawed. There's only one internal usage:

https://github.com/ReactTraining/history/blob/dca21eaa9230ae187c72ef657f70aac97cb172aa/modules/createHashHistory.js#L110

[timdorr]

Oh, I see why. Hash histories don't use key. I haven't been following those specifics too carefully, since I don't use those myself.

I would add a locationsAreEquivalent function in this case that checks pathname and search are equal.

[guidobouman]

Thanks for thinking along!

Looks like the key prop could be removed from the compare function, right? As hash history doesn't use the key prop anyhow. Then it would be usable in all places without adding an extra method.

[guidobouman]

Rewrote history a little. Not too smart for open-source, oops.

[guidobouman]

I see that semi-colons have been enabled in Prettier, I'll fix the conflicts.

[guidobouman]

Waiting for #559 before I can fix the conflicts in this branch.

[guidobouman]

@timdorr Ready for review again!

PS: What about the HashHistory warning?

[loganpowell]

subscribing to this as mentioned in #507 (comment)

[mjackson]

HashHistory emits a warning about replacing instead of pushing to the stack. But this is actually the WHATWG standards behaviour.

What do you mean @guidobouman? It's not possible to push the same path using hash history the way it is currently implemented.

Notice how the second window.location.hash = '/home' does not increase window.history.length. This means that pushing the same path using hash history does not create a new entry in the browser's history stack.

[mjackson]

I should also note that using pushState does push a new entry onto the history stack, even if the paths are the same.

If I understand you correctly, @guidobouman, are you saying it shouldn't?

[timdorr]

Perhaps we should fix this higher up in React Router's <Link> component? It's ultimately to fix a behavior to match <a> more closely.

[guidobouman]

Moving the discussion on this to #470, there's more historical info there.

[mjackson]

Based on the discussion in #470 I don't think we want to change the behavior of push since pushState does in fact push a new URL onto the history stack. Instead, the proposal is to add a new history.link API that behaves more like clicking on a link would do.

I'm going to close this PR, but thank you for taking the time to push this discussion forward @guidobouman :)

[guidobouman]

Always, @mjackson!

PS: I added some small bug fixes / tweaks in this PR, I'll put them up as different PR's.