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

Listen hashchange if popstate = false #542

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

PaulMaly
Copy link
Contributor

Seems we should add an event listener to hashchange if popstate option equals false and hashbang option equals true. In this case, right now back navigation via history.back() not working.

Seems we should add an event listener to `hashchange` if `popstate` option equals false and `hashbang` option equals true. In this case, right now back navigation via `history.back()` not working.
@coveralls
Copy link

coveralls commented Jun 19, 2019

Coverage Status

Coverage remained the same at 92.531% when pulling 502db29 on PaulMaly:patch-3 into 7f8e01d on visionmedia:master.

Seems the previous condition should be left completely. Only additional behavior needed.
@PaulMaly
Copy link
Contributor Author

PaulMaly commented Jun 25, 2019

@matthewp Any news here? Is this patch reasonable?

@matthewp
Copy link
Collaborator

It's hard to keep all of this in my head correctly. We've had a lot of modification of this part of the code. I'm a little bit afraid that we're playing bug whack-a-mole. We fix one thing but break something else. To prevent that going forward I feel we really need tests for each PR. So if you can add a test for this case then I think it's good to go.

@PaulMaly
Copy link
Contributor Author

PaulMaly commented Jul 2, 2019

@matthewp I believe it's a very obvious case to be tested. Seems we should trigger _onpopstate somehow any way in all possible cases. There's the case when _onpopstate never be triggered:

page.start({
    popstate: false,
    hashbang: true,
});

Because we've only two places where _onpopstate method could be triggered:

if(this._popstate)

and

if(this._hashbang && hasWindow && !hasHistory)

So, in a case when we've history and don't want to popstate, _onpopstate method will never be triggered.

It's very bad because if in this case, we'll press the browser back button, the router won't be informed about location changed.

@PaulMaly
Copy link
Contributor Author

@matthewp Any thoughts?

@matthewp
Copy link
Collaborator

Not on your last comment, no, sorry. But, what needs to happen is we need a test that verifies that a hashchange listener is added when { popstate: false }, that much makes sense to me.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants