walmartlabs / thorax

Strengthening your Backbone
http://thoraxjs.org/
Other
1.32k stars 129 forks source link

url helper and pushState #399

Closed bengladwell closed 9 years ago

bengladwell commented 9 years ago

I submitted PR #398. It is admittedly a confused PR. I should have opened an issue first. I'll close the PR since I'm not even really suggesting that solution anymore.

Does the issue raised in the PR make sense?

I originally commented: If you initialize Backbone.history with a root like "/path/" and pass a fragment like "/articles" to the url helper, you should get "/articles", not "/path/articles" [as a link href]. If you define a root, your route definitions do not include that root.

Later I realized that the root should be included in the hrefs so that you get proper hrefs that can be rendered by the server. In my code, I am addressing this by overriding _anchorClick like this:

  Thorax.View.prototype._anchorClick = function (event) {
    var target = $(event.currentTarget),
      href = target.attr('href');

    // Don't push if meta or shift key are clicked
    if (event.metaKey || event.shiftKey) {
      return true;
    }

    // Route anything that starts with # or / (excluding //domain urls)
    if (href && (href[0] === '#' || (href[0] === '/' && href[1] !== '/'))) {
      // everything about this _anchorClick should be the same as upstream except the following block
      if (Backbone.history.options.root.length > 1 && href.indexOf(Backbone.history.options.root) === 0) {
        href = '/' + href.substr(Backbone.history.options.root.length);
      }
      Backbone.history.navigate(href, {
        trigger: true
      });
      return false;
    }
    return true;
  };

This block:

if (Backbone.history.options.root.length > 1 && href.indexOf(Backbone.history.options.root) === 0) {
  href = '/' + href.substr(Backbone.history.options.root.length);
}

strips out the root before navigating if the href begins with the root.

Does this seem like a reasonable solution?

kpdecker commented 9 years ago

So the issue here is that navigate expects the root prefix to be removed but the links that are inserted into the html content do have this?

bengladwell commented 9 years ago

Yeah.

So you might have routes like:

routes: {
  "posts/": "posts"
}

If you initialize Backbone.history with a root like "rootpath", you do want the url helper to produce hrefs like href="/rootpath/posts/" so that your hrefs are correct for server-side rendering. However, when navigate tries to match /rootpath/posts/ against posts/, it will miss.

So, I'm suggesting that _anchorClick should strip the root before calling navigate when applicable.

bengladwell commented 9 years ago

Also, thanks for spending a good part of your Thanksgiving weekend working on Thorax. That's really awesome.

kpdecker commented 9 years ago

This should hopefully be fixed in the next release.