Strip fragment's search params before route matching in loadUrl

[mclin]

This is based on discussion with @braddunbar in #891

Based on my description he made this change: #2122
But that prevents the search params from getting into the history/url bar.

Here, just strip the search params for the purpose of route matching only.

[braddunbar]

Thanks for the patch @mclin! It's failing the tests though, and I'm not sure about this approach. If you want to ignore the search parameters, why do you need them in the url? Also, if the browser is using pushState then History#navigate will add the search parameters, but if hash based routing is in use then the "query" will just be appended to the fragment.

[mclin]

Hey, sorry. Didn't really mean this as a patch. Just wanted to show you what I meant. I'll look into how to run the tests.

I want to ignore the search params only when matching against routes, the same way that rails routing ignores search params.

But search params should accessible in the route handler, also same as rails, either as an extra param to the route handler or directly from window.location.search.

The url and the search params serve different purposes. The url identifies which resource you want (and hence which handler). The search params affect how it's queried (eg filter/sort/pagination) or how it's displayed (eg which tab in a tab view to show).

I've only used backbone with pushState on, I'll see how this would affect hash based routing.

[caseywebdev]

@mclin For future reference, on GitHub, if you just want to show a diff you can use the diff code type i.e. ```diff ...

diff --git a/backbone.js b/backbone.js
index 25ce414..bdd90f8 100644
--- a/backbone.js
+++ b/backbone.js
@@ -1026,6 +1026,7 @@

   // Cached regex for stripping a leading hash/slash and trailing space.
   var routeStripper = /^[#\/]|\s+$/g;
+  var searchStripper = /\?.*$/g;

   // Cached regex for stripping leading and trailing slashes.
   var rootStripper = /^\/+|\/+$/g;
@@ -1158,6 +1159,7 @@
     // returns `false`.
     loadUrl: function(fragmentOverride) {
       var fragment = this.fragment = this.getFragment(fragmentOverride);
+      fragment = fragment.replace(searchStripper, '');
       var matched = _.any(this.handlers, function(handler) {
         if (handler.route.test(fragment)) {
           handler.callback(fragment);

and GitHub will color it up nicely.

[braddunbar]

@caseywebdev Good tip!

[mclin]

@caseywebdev The more you know. Thanks!

[grydstedt]

+1, this would be very helpful.

[mclin]

Got all the tests passing. Also made it parse the search params in _extractParameters and pass them in the last param to the route handler as an Object.

[KODerFunk]

This is very important patch. I still waiting for it! Hope soon to be merged.

[mondaychen]

I've read the discussion in #891 and I believe this is a better way to do it.

But in my opinion, it's not Backbone's job to parse the search params and push it into the extracted parameters for the matched routes.

Maybe we can just add a Backbone.history.search to make search params accessible with or without pushState.

[existentialism]

+1, this is very similar to changes we've implemented minus the parameter parsing (would be handy, but I can see the argument against it not being Backbone's job).

[mclin]

Hey, I see your point about not parsing the search string, so I took it out.

[mondaychen]

@mclin 👍 That's better :)
But still I'm not sure whether it is the best way to do that with putting a extra parameter into the routes functions -- though I'm doing the same way currently.
Well, maybe it's better to explain with code.
Here's something from my project. I hacked the _routeToRegExp function the make it strip the location.search query.

var ParamsStripper = Backbone.Router.extend({
    constructor: function () {
      var oldRegCreator = this._routeToRegExp
      this._routeToRegExp = function(route) {
        var reg = oldRegCreator.call(this, route)
        return new RegExp(reg.source.replace(/\$$/, '([\\?]{1}.*)?$'))
      }
      Backbone.Router.apply(this, arguments)
    }
  })

  var AppRouter = ParamsStripper.extend({
    routes: {
      '': 'home',
      'category/:category(/)': 'category',
      'tag/:tag(/)': 'tag',
      '*actions': 'redirectToHome'
    }
...

And this is the reason against the extra parameter

    tag: function(tag, query, href) {
      href = href || '/tag/' + tag
      var pageTag = new PageTag(tag, href)
    },
    category: function(category, query) {
      query = decodeURI(query || location.search)
      // this url sometimes use the `tag` page ui
      if(category === 'fiction' || category === 'nonfiction') {
        this.tag(category, query, '/category/' + category)
        return
      }
      var pageCategory = new PageCategory(category, query)
    },

The 2nd param in the tag function is actually useless but I have to leave it there.

[mateusmaso]

+1 for it.. I'm facing the same problem of route matching when there is search query appended in url, this pull solved my problem and I hope it will be soon at master

[mateusmaso]

just noticed that when backbone.js first load no query string is placed inside History fragment

getFragment: function(fragment, forcePushState) {
      if (fragment == null) {
        if (this._hasPushState || !this._wantsHashChange || forcePushState) {
          fragment = this.location.pathname;

as you can see, getFragment method is ignoring if there is no fragment parameter passed inside the function (the case when you first load your app)... my solution is simple and its working for now as a workaround

fragment = this.location.pathname + this.location.search;

[mclin]

Hey @mateusmaso, that change is already in this diff.

[eastridge]

+1 Seems like the intuitive thing to do.

[jashkenas]

@braddunbar -- do you consider this PR to supersede yours? Let me know.

[jashkenas]

Backbone shouldn't be messing with the search params, as they don't have a valid semantic meaning from the point of view of a Backbone app. If you want to use them (on a page that has a running backbone app), that's totally fine ... but you don't need to pass them to navigate. Pasting in the conversation from IRC:

braddunbar> So the query stripping...
•jashkenas> indeed. which of those two patches is the way to go?
braddunbar> The question is, should #navigate be adding query parameters?
•jashkenas> Adding back ones that are already present on the page?
•jashkenas> as opposed to stripping them?
braddunbar> Something like Backbone.history.navigate('my/fragment?my=option')
braddunbar> Currently, it doesn't match anything because we ignore search params.
braddunbar> ...but it's been proposed that we add special machinery to allow it to be set in the url without attempting to match it.
•jashkenas> Huh. I was thinking more like we should patch it to transparently ignore search params.
braddunbar> Exaclty.
braddunbar> Exactly.*
•jashkenas> So if you start a backbone app on a page that has params.
•jashkenas> We just operate with a ?params#fragment at the end, or with a /fragment?params at the beginning.
•jashkenas> Pretending like they're not there, but not adding or removing them.
•jashkenas> Is that our current behavior?
•jashkenas> (and not trying to match against them)
braddunbar> I think so, let me double check.
braddunbar> Yes, we ignore it by using location.pathname.
braddunbar> However, that's not exactly the issue.
braddunbar> What's proposed is that we allow it to be changed via #navigate.
braddunbar> ...with special machinery to ignore it when routing.
braddunbar> I would prefer to stay away from it.
•jashkenas> yep. let's stay away
•jashkenas> you can change it yourself
•jashkenas> you don't need navigate to do that
braddunbar nods
•jashkenas> so just close the two tickets?
braddunbar> Yep, and just don't pass search params to #navigate.
braddunbar> Stripping them is ok with me too, but I don't have strong feelings either way.

[hitchcockwill]

@jashkenas is the expected behavior in 1.0.0 that router.navigate will ignore question marks and anything following them?

For example, should router.navigate('test/?foo=bar') navigate to test/ ?

I'm finding this to be the case when doing a complete page reload, but not when triggering a route change with pushState.

[braddunbar]

Mornin' @hitchcockwill! No, Router#navigate will not strip the query params for you. It's assumed that the fragment you pass in does not include search params.

[hitchcockwill]

@braddunbar thanks for the reply. Is it expected that test/?foo=bar would route to test/ after a page refresh? Was this a conscious design choice? This seems like inconsistant behavior.

[braddunbar]

That's right, Backbone.History always ignores the query string when getting the fragment from window.location.

[hitchcockwill]

Is there a reason that the router behaves like this? I know that this has been a topic of debate over the last few months, but the current solution seems half-baked. If the sentiment is that Backbone should ignore query parameters, then shouldn't Backbone.History and the Router#navigate function both act in the same way?

[braddunbar]

Check out #891 for some background. It holds most of the conversation surrounding search params and the problems surrounding them. As for #navigate and stripping parameters, I'm fine with either way as I say above. That said, it's easy enough to remove the query string before passing a fragment to #navigate (.replace(/\?.*$/, '')).