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

pushState + hash on root URL #21

Closed
tkhyn opened this issue Dec 1, 2015 · 6 comments

Comments

Projects
None yet
3 participants
@tkhyn
Copy link

commented Dec 1, 2015

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

This comment has been minimized.

Copy link
Member

commented Dec 1, 2015

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

@tkhyn

This comment has been minimized.

Copy link
Author

commented Dec 1, 2015

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

This comment has been minimized.

Copy link
Member

commented Dec 1, 2015

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

This comment has been minimized.

Copy link
Member

commented Dec 1, 2015

@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

This comment has been minimized.

Copy link
Author

commented Dec 1, 2015

@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

This comment has been minimized.

Copy link
Member

commented Jul 4, 2016

Closing this since it's a duplicate.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.