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

Optional trailing slash #2656

Closed
joshpangell opened this issue Jul 4, 2013 · 13 comments
Closed

Optional trailing slash #2656

joshpangell opened this issue Jul 4, 2013 · 13 comments
Labels

Comments

@joshpangell
Copy link

When defining a root route, then pushing history onto the stack, then removing it, there is a trailing slash left over. This is not very desirable and would be great as an optional flag when starting history.

The origin of this appears to reside in the code here.

As an example:

Backbone.history.start({
    pushState: true,
    root: "/myroot"
});

Opening a page and pushing it onto the history stack works.

var url = "/Page-name";
Router.navigate( url, { trigger: true });

Result: example.com/myroot/Page-name (this is the desired result)

Then, when closing the page, passing a blank value to remove the page url creates a trailing slash:

var url = "";
Router.navigate( url, { trigger: true });

Result: example.com/myroot/

Desired result: example.com/myroot (no trailing slash)

@jashkenas
Copy link
Owner

Pinging @braddunbar, in particular about these two lines:

// Normalize root to always include a leading and trailing slash.
this.root = ('/' + this.root + '/').replace(rootStripper, '/');

... did you have a definite opinion at the time the change was made about root URLs (without additional components) needing to include the trailing slash? Or was that just a side effect of assuming that roots will usually only be a prefix for additional URL components?

@braddunbar
Copy link
Collaborator

Definitely the latter. I don't have a strong opinion on trailing slashes on the root, though I'm not aware of any problems with it.

@joshpangell What problem is the trailing slash causing? Is it purely aesthetic or does it cause a technical issue?

@jashkenas
Copy link
Owner

Then let's assume that "pure" root URLs never have a trailing slash put on by us ... although perhaps we allow you to .navigate to one with a slash if you do so manually. Resource-wise, it's best for us to not assume the old-school apache-slash-means-index.html idiom.

@braddunbar
Copy link
Collaborator

Sounds good, patch incoming.

@joshpangell
Copy link
Author

@braddunbar The trailing slash is not causing a technical issue per say, but aesthetics are very important :)

@braddunbar
Copy link
Collaborator

I addressed this in #2659, but I'm not sure I like the solution. For one, I'm uncomfortable fiddling with such a finicky portion of the code for something that's not currently causing any problems. And for two, there are some caveats, as described in the pull. These could possibly be addressed, but it seems like a lot of trouble for a trailing slash.

@joshpangell
Copy link
Author

A potential caveat is cache. Many people cache based on URL, especially with Varnish. Slashes in the URL are looked at as different locations, when in fact they are not.

@jashkenas
Copy link
Owner

but I'm not sure I like the solution

Right-o. Instead of this fiddling, can we simply normalize to never include the slash directly on the root?

@braddunbar
Copy link
Collaborator

Sure thing, updated in 151bd73.

braddunbar added a commit that referenced this issue Jul 6, 2013
@flvngrvllt
Copy link

I'm developing a WordPress theme based on Backbone and using pretty permalinks as http://example.com/multi-site-name/yyyy/mm/dd/post-name/. Assuming the root route is /multi-site-name/, automatically remove trailing slash is a problem in this case 'cause home page url becomes http://example.com/multi-site-name instead of http://example.com/multi-site-name/ and if I refresh my web browser when I am on http://example.com/multi-site-name, I have a loading extra time 'cause WordPress redirects to http://example.com/multi-site-name/

@pawelgur
Copy link

pawelgur commented Jan 8, 2014

Same as @Deknar here.
We are using [only] trailing slashes everywhere (for the sake of SEO/duplicate pages in analytics).
Now (1.1.0) backbone always removes trailing slash from root even if using router.navigate("/") which is really undesirable.
This way Backbone forces to use url's without trailing slashes, this must be optional (when explicitly using router.navigate("/") slash shouldn't be removed ).

@kyse
Copy link

kyse commented Jun 11, 2014

I know this is kind of old, but since I found this in a search I figured I'd post my solution as I haven't found any updates otherwise to allow us to specify optionally how we want it handled. The removal of the slash was causing my identity solution to issue relogin requests every time I hit the home route without the trailing slash.

Notes:
-I took this route versus editing the Backbone js directly in an effort to keep it playing nicely when backbone is downloaded via a Nuget package in the solution.

-I used the minified version for the most part and changed History.started to Backbone.History.started and the call for pathStripper needed to reference Backbone.pathStripper since this is being called outside the class.

Just place this somewhere in your JS after the dom is ready.

_.extend(Backbone.history, {
        navigate: function (t, e) { if (!Backbone.History.started) return false; if (!e || e === true) e = { trigger: !!e }; var i = this.root + (t = this.getFragment(t || "")); t = t.replace(Backbone.pathStripper, ""); if (this.fragment === t) return; this.fragment = t; if (this._hasPushState) { this.history[e.replace ? "replaceState" : "pushState"]({}, document.title, i) } else if (this._wantsHashChange) { this._updateHash(this.location, t, e.replace); if (this.iframe && t !== this.getFragment(this.getHash(this.iframe))) { if (!e.replace) this.iframe.document.open().close(); this._updateHash(this.iframe.location, t, e.replace) } } else { return this.location.assign(i) } if (e.trigger) return this.loadUrl(t) }
    });

@akre54
Copy link
Collaborator

akre54 commented Jun 13, 2014

Hey @kyse, how about the solution from #3136? Would that work for your use case?

I'd advise against adding monkey patches with minfied code. Think what a nightmare that'd be to maintain!

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

No branches or pull requests

7 participants