Merge lp:~bcsaller/juju-gui/routing-hash-query into lp:juju-gui/experimental

[Benjamin Saller]

Better Hash/QS support in router

`parse`, `url` and `combine` deal more gracefully with hash and qs url
options. In the case of combine the incoming url replaces any existing
values in the hash and qs portions of the URL as might be expected. (This
is rather than some artificial merge policy for example).

https://codereview.appspot.com/9830046/

[Benjamin Saller (bcsaller)]

Reviewers: mp+168351_code.launchpad.net,

Message:
Please take a look.

Description:
Better Hash/QS support in router

`parse`, `url` and `combine` deal more gracefully with hash and qs url
options. In the case of combine the incoming url replaces any existing
values in the hash and qs portions of the URL as might be expected.
(This
is rather than some artificial merge policy for example).

https://code.launchpad.net/~bcsaller/juju-gui/routing-hash-query/+merge/168351

(do not edit description out of merge proposal)

Please review this at https://codereview.appspot.com/9830046/

Affected files:
   A [revision details]
   M app/assets/javascripts/ns-routing-app-extension.js
   M test/test_routing.js

[Richard Harding (rharding)]

LGTM with a concern about the hash parsing to test/check on please.

https://codereview.appspot.com/9830046/diff/1/app/assets/javascripts/ns-routing-app-extension.js
File app/assets/javascripts/ns-routing-app-extension.js (right):

https://codereview.appspot.com/9830046/diff/1/app/assets/javascripts/ns-routing-app-extension.js#newcode111
app/assets/javascripts/ns-routing-app-extension.js:111: var match =
url.match(/#(\w+)([?$])?/);
Can you test that this matches anchor tags with - in them? I don't
recall if \w will work for that. In our case, we've 'namespaced' our
anchors in our tabs code #bws-configuration for instance.

Since there's only one hash per url can we just directly check
location.hash and let the browser do the logic?

https://codereview.appspot.com/9830046/

[j.c.sackett (jcsackett)]

LGTM, with one question about using a regex over split, largely for my
own edification.

Thanks for this branch, Ben.

https://codereview.appspot.com/9830046/diff/1/app/assets/javascripts/ns-routing-app-extension.js
File app/assets/javascripts/ns-routing-app-extension.js (right):

https://codereview.appspot.com/9830046/diff/1/app/assets/javascripts/ns-routing-app-extension.js#newcode111
app/assets/javascripts/ns-routing-app-extension.js:111: var match =
url.match(/#(\w+)([?$])?/);
Is this actually better than just splitting on '#' much as getQS splits
on '?' Provided Rick's concerns are addressed, I don't know that it
matters, but regexing seems overkill for this.

https://codereview.appspot.com/9830046/

[lp:~bcsaller/juju-gui/routing-hash-query]

714. By Benjamin Saller on 2013-06-10

change hash to split rather than regex style code

715. By Benjamin Saller on 2013-06-10

change hash to split rather than regex style code

[Benjamin Saller (bcsaller)]

Please take a look.

https://codereview.appspot.com/9830046/

Unmerged revisions

715. By Benjamin Saller on 2013-06-10

change hash to split rather than regex style code

714. By Benjamin Saller on 2013-06-10

change hash to split rather than regex style code

=== modified file 'app/assets/javascripts/ns-routing-app-extension.js'
--- app/assets/javascripts/ns-routing-app-extension.js	2013-05-22 18:47:13 +0000
+++ app/assets/javascripts/ns-routing-app-extension.js	2013-06-10 22:11:26 +0000
@@ -61,9 +61,29 @@
     return result;
   }
 
-  var Routes = {
-    pairs: function() {return pairs(this);}
-  };
+  var Routes = (function() {
+    function Routes() {
+      return Object.create(this, {
+        defaultNamespacePresent: {
+          enumerable: false,
+          writable: true
+        },
+        hash: {
+          enumerable: false,
+          writable: true
+        },
+        search: {
+          enumerable: false,
+          writable: true
+        },
+        pairs: {
+          enumable: false,
+          value: function() {return pairs(this);}
+        }
+      });
+    }
+    return Routes;
+  })();
 
   // Multi dimensional router (TARDIS).
   var _Router = {
@@ -81,6 +101,25 @@
     },
 
     /**
+      Return the #hash portion of the URL if present.
+
+      @method getHash
+      @param {String} url to parse hash from.
+      @return {String} hash || null.
+    */
+    getHash: function(url) {
+      var idx = url.indexOf('#');
+      if (idx === -1) {
+        return null;
+      }
+      var end = url.indexOf('?');
+      if (end === -1) {
+        end = url.length;
+      }
+      return url.slice(idx + 1, end);
+    },
+
+    /**
      * Split a URL into components, a subset of the
      * Location Object.
      *
@@ -94,6 +133,7 @@
       };
       var origin = this._regexUrlOrigin.exec(url);
       result.search = this.getQS(url);
+      result.hash = this.getHash(url);
 
       if (origin) {
         // Take the match.
@@ -127,20 +167,23 @@
      **/
     parse: function(url, combineFlags) {
       combineFlags = Y.mix(this.combineFlags || {}, combineFlags, true);
-      var result = Object.create(Routes, {
-        defaultNamespacePresent: {
-          enumerable: false,
-          writable: true
-        }
-      });
+      var result = new Routes();
       var parts = this.split(url);
       url = parts.pathname;
+      result.hash = parts.hash;
+      result.search = parts.search;
 
+      // If there was a hash we need to split it off url
+      if (result.hash) {
+        url = url.slice(0, url.indexOf('#'));
+      }
       parts = url.split(this._fragment);
-      //  > '/foo/bar'.split(this._fragment)
-      //    ["/foo/bar"]
-      //  > :baz:/foo/bar'.split(this._fragment)
-      //    ["", ":baz:", "/foo/bar"]
+      // Example output
+      // '/foo/bar'.split(this._fragment)
+      // ["/foo/bar"]
+      //
+      // ':baz:/foo/bar'.split(this._fragment)
+      // ["", ":baz:", "/foo/bar"]
       if (parts[0]) {
         // This is a URL fragment without a namespace.
         parts[0] = rtrim(parts[0], '/') + '/';
@@ -225,11 +268,20 @@
       });
 
       url = slash(url);
+
+      if (components.hash) {
+        url += '#' + components.hash;
+      }
+
+      if (components.search) {
+        url += '?' + components.search;
+      }
       return url;
     },
 
     /**
-     * Smartly combine new namespaced url components with old.
+     * Smartly combine new namespaced url components with old. Hash and
+     * query string parameters from the incoming URL are preserved.
      *
      * @method combine
      * @param {Object} orig url.
@@ -260,7 +312,7 @@
         // original value).
         delete incoming[this.defaultNamespace];
       }
-      var output = {};
+      var output = new Routes();
       Y.each(orig, function(v, k) {
         if (v && !Y.Lang.isArray(v)) {
           v = [v];
@@ -290,9 +342,12 @@
           }
         });
       });
+      // TODO: Need combine support for hash/search. Output can be a Routes
+      // object to make this work as hash/search wouldn't be in the enum set.
+      output.hash = incoming.hash;
+      output.search = incoming.search;
       url = this.url(output, {excludeRootPaths: true});
       return url;
-
     }
   };
 

=== modified file 'test/test_routing.js'
--- test/test_routing.js	2013-05-23 15:46:35 +0000
+++ test/test_routing.js	2013-06-10 22:11:26 +0000
@@ -90,6 +90,15 @@
     assert(u === '/:a:alpha/:b:beta/:gamma:g/');
   });
 
+  it('should support hashes and query strings', function() {
+    var router = juju.Router('test');
+    var url, parts;
+    parts = router.parse('/foo/#bar?baz=something+else&battery=acid');
+    assert.equal(parts.hash, 'bar');
+    assert.equal(parts.search, 'baz=something+else&battery=acid');
+    assert.equal(parts.test, '/foo/');
+  });
+
   it('should support a default namespace', function() {
     var router = juju.Router('charmstore');
     var url, parts;
@@ -115,6 +124,15 @@
        url.should.equal('/foo/bar/');
        url = router.combine('/foo/bar', '/:inspector:/foo/');
        url.should.equal('/foo/bar/:inspector:/foo/');
+
+       // Hash and querystrings come from the second (incoming)
+       // argument to combine, values from the original url
+       // are discarded.
+       url = router.combine('/foo/bar?world=gone+away',
+                            '/:inspector:/foo/#hello?world=beater');
+       url.should.equal('/foo/bar/:inspector:/foo/#hello?world=beater');
+
+
      });
 
   it('should be able to split qualified urls', function() {