aurelia / history-browser

An implementation of the history interface based on standard browser hash change and push state mechanisms.
MIT License
23 stars 29 forks source link

pushState + hash on root URL #21

Closed tkhyn closed 8 years ago

tkhyn commented 8 years ago

Hi (and big thanks already for all the great work on Aurelia),

I've started to play around with the framework by setting up an app which router is configured with config.options.pushState = true; to use pretty URLs.

I would still like to be able to use bookmarks on my root page (such as http://host/#bookmark), and I noticed that the location was systematically turned into http://host/bookmark. # is however not stripped when using an URL other than the root location (i.e. http://host/whatever#bookmark works fine).

This seems to happen here.

Is it the expected behavior to mutually exclude bookmarks or routing on the root URL when using pushState = true ? If that's the case I'll challenge that, I think!

EisenbergEffect commented 8 years ago

I think it's likely just a use case we didn't initially consider. @bryanrsmith can comment on this more though.

tkhyn commented 8 years ago

Having a closer look at the source code, I set config.options.hashChange = false; and it works perfectly.

So the only thing that'd be missing here is a mention of hashChange somewhere in docs (knowing of course that it's a work in progress).

Or I may suggest something that could be more sensible (but I don't know all the in and outs and it may break other things) :

diff --git a/src/index.js b/src/index.js
index aee7c41..24f20e2 100644
--- a/src/index.js
+++ b/src/index.js
@@ -51,8 +51,8 @@ export class BrowserHistory extends History {
     // Normalize root to always include a leading and trailing slash.
     this.root = ('/' + this.options.root + '/').replace(rootStripper, '/');

-    this._wantsHashChange = this.options.hashChange !== false;
     this._hasPushState = !!(this.options.pushState && this.history && this.history.pushState);
+    this._wantsHashChange = (this.options.hashChange === undefined ? !this._hasPushState : !!this.options.hashChange)

     // Determine how we check the URL state.
     let eventName;

(as I'm still pretty new to Aurelia / Karma / jspm and even Node.js, I struggled to find a reliable way to test the activate() method. Is there an example somewhere in an Aurelia module where something similar is done?)

bryanrsmith commented 8 years ago

I think this is a duplicate of aurelia/router#225. As far as I know it was indeed an oversight. I agree with your proposal, @tkhyn, to change the default behavior so that the router must be explicitly configured to rewrite incoming hashes as paths.

This is technically a breaking change for applications that want incoming hash-based fragments to be transformed, but it seems unlikely to affect many users (see http://caniuse.com/#feat=history), and the migration is trivial.

bryanrsmith commented 8 years ago

@tkhyn I'm not sure of a good reference for testing the activate method-- they're a bit lacking at the moment. Maybe some of the tests in aurelia/router? Unfortunately you'll probably need to just read the code and build tests around it. This would be a very welcome contribution, if you're up for it! Feel free to ping me here or on gitter if there are any specifics I can help with.

tkhyn commented 8 years ago

@bryanrsmith aurelia-router's tests use a simple History mock class which does nothing. That's the approach I had started to follow but I saw it led to fat ugly mock classes and I thought you guys would know a better way! If I have the time to push it further I'll open another issue (or submit a PR if I manage to do something clean enough).

Regarding the issue, I'll leave you amend the code when it suits. For the moment I'm quite happy with hashChange = false!

EisenbergEffect commented 8 years ago

Closing this since it's a duplicate.