visionmedia / page.js

Micro client-side router inspired by the Express router
http://visionmedia.github.com/page.js
7.67k stars 687 forks source link

Listen `hashchange` if popstate = false #542

Open PaulMaly opened 5 years ago

PaulMaly commented 5 years ago

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 commented 5 years ago

Coverage Status

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

PaulMaly commented 5 years ago

@matthewp Any news here? Is this patch reasonable?

matthewp commented 5 years ago

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 commented 5 years ago

@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 commented 4 years ago

@matthewp Any thoughts?

matthewp commented 4 years ago

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.