Only handle hashes with a '/' when using createHashHistory

[cwrs]

This PR adds the ability to ignore hash changes when using createHashHistory().

Example:

function ignorePath(path){
   return path.startWidth('ignore-me');
}
let history = createHistory({'ignorePath': ignorePath});

We have the issue, that the hashChangeListener calls ensureSlash for hashes, not handled by the history.

[taion]

Can you speak a little to the use case you're trying to handle here?

[cwrs]

Our Redux-Application runs within another single-page application (GWT) that handles history via hashes itself. In our setup the outer application only handles specific hashes that start with '!'. When a user of the inner application clicks on a link with a '!' at the beginning to reach a page of the outer application, the hashChangeListener prepends the '/' by calling ensureSlash() and destroys the hash for the outer application.

[taion]

Huh. This seems a little odd to me.

By the same reasoning, if you were using two separate HTML5 routers, would you also want to ignore certain paths? I'm not sure it makes sense to add something unintuitive like this for supporting multiple routers trying to work off the same global location.

Or rather, this change in this place does not seem to match the use case you describe.

[cwrs]

You're right, I would prefer to have one global history handler and I guess its not a common use case.
For the moment a user switches from one app to the other, we must prevent the hashChangeListener to call ensureSlash(). For us it would help a lot, if it would be possible to override or configure the ensureSlash() function within the createHashHistory module. I thought a ignore-Path function would be a more common use case.

[mjackson]

So, you want a way for hash history to ignore all changes? Or just some changes? i.e. Is there ever going to be a case when you:

function ignorePath() {
  return true
}

?

[cwrs]

I want the hashHistory to ignore all hashes that don't start with a '/'. For all other hashes I would return true.
Currently it automatically adds the '/' whenever a hash is not recognized as an absolute path.

[taion]

This comes up in the context of social auth as well - some providers will use hash to provide auth information to the client application.

Would it make more sense to have createHashHistory just ignore all hashes that don't start with / globally? It seems like it'd fix both your use case and the social auth use case transparently, while not forcing users to do additional configuration.

I don't think we ever intentionally generate hashes that don't start with /, do we?

[cwrs]

It would definitely match our use case, but it might destroy 404-handling for others I guess? But prepending a '/' on history-change might as well. I think just removing the ensureSlash-call on historyChange might work for both use cases as well?

[taion]

I feel like if all of our paths start with /, it's just an error condition if you push a hash without it.

[taion]

@cwrs What do you want to do here? Do you think it'd be too intrusive to always ignore paths that don't start with /?

[cwrs]

I think its the right thing to do. I could update my pull-request next week. Sorry for answering so late, I'm on vacation right now.

[taion]

No worries, just wanted to follow up.

[cwrs]

I changed my pull-request, but I'm not sure, how to test the new behaviour...

[taion]

Listen to the hash history with a spy, and count how many times that spy gets called.

[cwrs]

I tried to write a test and integrated SinonJs for the spy. Testing the behavour directly in the browser works, but my test will not work. Am I calling the wrong methods?

[taion]

A little confusingly, expect already has the capacity for creating spies. We'd prefer you use that for consistency.

[taion]

Specifically: https://github.com/mjackson/expect#createspy

[cwrs]

Thx, that was unexpected :). Much easier than integrating sinon.

[cwrs]

The tests work fine for me in Chrome... Do you have an idea what's wrong?

[ghost]

I have a similar problem with oauth.

When auth0 is using the provided callback, it usually looks something like this: https://webiste.com/#access_token=abc

react-router interpret that as https://webiste.com/#/access_token=abc and complains that there is no route access_token.

Is there any way around that yet?

[taion]

@cwrs The failures were in non-Chrome browsers on BrowserStack. At a glance, looks like not a false positive.

[cwrs]

@taion thx, and sorry for answering so late. I will update my patch to the current version and see, if I can reproduce the errors on other browsers.

[cwrs]

My tests are now working, but I didn't change a thing since.

[taion]

Are these and other changes needed?

[taion]

I mean, the ones of history -> this.

[cwrs]

The changes are only needed for tests, to use a spy to check of transitionTo is called.

[taion]

This is actually not enough to fix e.g. remix-run/react-router#2776.

The problem is that getCurrentLocation() for the initial location after the redirect is still going to fire with the "incorrect" location.

We might have to handle this some other way.

[ryanflorence]

Seems like we should only handle changes that came from our API.

history.push(...)

// internally
handlingWithHistory = true

// then in the hashchange listener
if (handlingWithHistory)
  doStuff()

// then switch it back
handlingWithHistory = false

Then we'd be ignoring any hash changes not initiated by the library. I think paths shouldn't matter, but how the change was initiated that matters.

[taion]

@ryanflorence

We need to deal with POP actions, though, as well as the initial hash location when the user first hits the page – neither of those are triggered directly by the history.

The PR actually gets us a good part of the way there – the problem is that in the case of getting redirected to e.g. my/page#auth_token=whatever by a third-party service, we still need to handle the case where the initial location is not "valid" in some sense.

[cwrs]

would it be possible to merge my PR if I update it to master? Or should I try to handle the initial location issue?

[taion]

@cwrs Do you have a good idea for handling the initial position issue? My initial thought is that it'd be better handled by the router than by history.

[taion]

@cwrs Actually, does it help your use case at all if we just drop the ensureSlash behavior? Like, get rid of that function and drop everything that references it? You might still get some React Router warnings, but those shouldn't break you. We can then maybe find a better way to deal with this on the Router side if needed.

[cwrs]

I'm currently testing a history-version without the ensureSlash() in our app. I was about to ask, if we can't drop the ensureSlash myself. All history-tests run well. So I'm totally fine with that.

[cwrs]

The version without emsureSlash is running in production for us now for about 4 weeks without problems. I rebased my patch to the current master.

[taion]

ping @mjackson

[cwrs]

I guess with the new hashType option (6be82b0) this pull-request is now obsolete.

[taion]

Thanks for putting up this PR.

[cwrs]

Thx for all your great work!