Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Already on GitHub? Sign in to your account

Normalize fragment when hashChange: false in navigate #1206

Merged
merged 2 commits into from Apr 10, 2012

Conversation

Projects
None yet
3 participants
Contributor

aterris commented Apr 10, 2012

I am running on master for a project so I can take advantage of the change from PR #1185. However, today I realized I was having an issue with navigate on that project.

I franticly scrambled thinking that my pull request was actually broken, but realized it was just my own user error in my app. I was calling navigate with a leading slash, which got the root ('/') added in front for '//item/slug'. when the browser did not have pushState and had hashChange: false this bad url would be sent to window.location.assign, which causes incorrect behavior because of the leading double slash.

As I said, this is actually user error, and removing the leading slash causes it to function as expected. However, I noticed that when using pushState, there is a short idiom using frag to deal with this (which is why the problem only occurs in non pushState browsers)

The attached change uses that same pattern to protect this case as well

@braddunbar braddunbar and 1 other commented on an outdated diff Apr 10, 2012

backbone.js
@@ -1119,7 +1119,9 @@
// If you've told us that you explicitly don't want fallback hashchange-
// based history, then `navigate` becomes a page refresh.
} else {
- window.location.assign(this.options.root + fragment);
+ if (frag.indexOf(this.options.root) != 0) frag = this.options.root + frag;
+ this.fragment = frag;
@braddunbar

braddunbar Apr 10, 2012

Collaborator

I believe this line could be removed. Setting this.fragment won't matter since we're immediately navigating to another location right?

@aterris

aterris Apr 10, 2012

Contributor

Yep, definitely.

Contributor

aterris commented Apr 10, 2012

I am actually running into a weird related issue. When using this code, things mostly work as expected. However, intermittently it seems to "double load". We are using trigger: true in this case and it almost seems like it calls loadUrl and hits the router before it actually loads the new page.

It doesn't seem like the loadUrl call should be able to occur since its after window.location.assign is called (it should just load the new page) but adding a return on line 1124 seemed to resolve the issue and stop the double loading.

  • IE8 and IE9

I believe this issue will be present even without the change in this pull request, but I still have to confirm that, will be doing that now.

Contributor

aterris commented Apr 10, 2012

I just checked this issue and I seem to be encountering it even on master. I still cant find anything about window.location.assign being delayed so I dont really see how loadUrl can actually be called, but adding a return there on 1124 seems to address that issue.

I am going to update this for your comment and this change, but I think I need to figure out why this works before I am confident getting this merged in

@jashkenas jashkenas added a commit that referenced this pull request Apr 10, 2012

@jashkenas jashkenas Merge pull request #1206 from aterris/frag
Normalize fragment when hashChange: false in navigate
656f59e

@jashkenas jashkenas merged commit 656f59e into jashkenas:master Apr 10, 2012

@jashkenas jashkenas added a commit that referenced this pull request Apr 10, 2012

@jashkenas jashkenas simple refactor for #1206 67c3ea8
Contributor

aterris commented Apr 10, 2012

good call on drying that up a bit for me, thanks 👍

@braddunbar braddunbar added a commit to braddunbar/backbone that referenced this pull request Jun 13, 2012

@braddunbar braddunbar Add router tests for #1185, #1206. e4caafc

@jashkenas jashkenas added a commit that referenced this pull request Jun 13, 2012

@jashkenas jashkenas Merge pull request #1404 from braddunbar/history-tests
Add router tests for #1185, #1206.
2cd5818
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment