fix handling of array like query params (#114)

After migrating to URLSearchParams, we introduced a bug where query
strings like `?a=1&a=2&a=3` would result in `{ a: '1' }` instead
of `{ a: ['1', '2', '3'] }`. This commit fixes this issue.

CHANGELOG.md

@@ -1,18 +1,21 @@
 # Change Log
 
+## 3.0.1
+
+-   [#114] fix handling of array like query params
+
 ## 3.0.0
 
 ### Breaking Changes
 
--   `routr` uses native `URLSearchParams` instead of `query-string`
-    library internally. If you need to support old browsers, you can
-    either add a `URLSearchParams` polyfill or inject `query-string`
-    when instantiating `routr`:
+-   [#110] `routr` uses native `URLSearchParams` instead of `query-string` to parse query strings. As a consequence, parsing `?foo` will result in `{ foo: '' }` as specified in the [WHATWG spec](https://url.spec.whatwg.org/#interface-urlsearchparams) instead of `{ foo: null }` as `query-string` would do. Also, `URLSearchParams` is not available in older browsers (noticeably IE11). If you need to support them, you can either add a `URLSearchParams` polyfill or inject `query-string` when instantiating `routr`:
+
 ```js
 router = new Routr(routes, {
     queryLib: require('query-string'),
 });
 ```
+
 -   [#113] Updated `path-to-regexp` to its latest version
 
 ## 2.1.0
@@ -43,9 +46,9 @@ router = new Routr(routes, {
 ### Features
 
 -   [#30] Route definitions should now be defined as an array of route objects
-    rather than a map of routes. The old method of defining routes with a map
-    is still supported, but ordering can not be guaranteed (as per the JavaScript
-    engine's implementation).
+    rather than a map of routes. The old method of defining routes
+    with a map is still supported, but ordering can not be guaranteed
+    (as per the JavaScript engine's implementation).
 -   [#31] Added support for parsing and constructing urls with query strings.
     Matched route objects now contain a `query` property containing the map of
     query parameters. `router.makePath` now accepts a third `query` parameter

lib/router.js

@@ -10,11 +10,21 @@ var cachedCompilers = {};
 
 var queryString = {
     parse: function (string) {
-        var obj = {};
+        var obj = Object.create(null);
 
         var params = new URLSearchParams(string);
         params.forEach(function (value, key) {
-            obj[key] = value;
+            if (key in obj) {
+                return;
+            }
+
+            var values = params.getAll(key);
+
+            if (values.length > 1) {
+                obj[key] = values;
+            } else {
+                obj[key] = values[0];
+            }
         });
 
         return obj;
@@ -23,7 +33,12 @@ var queryString = {
         var params = new URLSearchParams();
         for (var key in obj) {
             if (Object.prototype.hasOwnProperty.call(obj, key)) {
-                params.append(key, obj[key]);
+                var value = obj[key];
+                if (Array.isArray(value)) {
+                    value.forEach((v) => params.append(key, v));
+                } else {
+                    params.append(key, value);
+                }
             }
         }
         params.sort();

tests/unit/lib/router.js

@@ -370,6 +370,15 @@ describe('Router', function () {
             expect(route.query).to.deep.equal({ foo: 'bar' });
         });
 
+        it('can parse query params correctly', () => {
+            const route = router.getRoute('/?foo=bar&a=b&a=c&bool');
+            expect(route.query).to.deep.equal({
+                foo: 'bar',
+                a: ['b', 'c'],
+                bool: '',
+            });
+        });
+
         it('method should be case-insensitive and defaults to get', function () {
             var route = router.getRoute(
                 '/finance/news/e-t-initially-horror-film-202700630.html'
@@ -476,9 +485,11 @@ describe('Router', function () {
 
     describe('#makePath', function () {
         var router;
+
         beforeEach(function () {
             router = new Router(routesArray);
         });
+
         it('existing route', function () {
             var path = router.makePath('article', {
                 site: 'SITE',
@@ -488,6 +499,7 @@ describe('Router', function () {
             });
             expect(path).to.equal('/SITE/CATEGORY/SUBCATEGORY/ALIAS.html');
         });
+
         it('handle optional params', function () {
             var path = router.makePath('article', {
                 site: 'SITE',
@@ -501,6 +513,7 @@ describe('Router', function () {
             });
             expect(path).to.equal('/SITE/ALIAS.html');
         });
+
         it('handle custom match params', function () {
             var path = router.makePath('custom_match_params', {
                 id: '12345',
@@ -511,27 +524,36 @@ describe('Router', function () {
             });
             expect(path).to.equal(null);
         });
+
         it('handle unamed params', function () {
             var path = router.makePath('unamed_params', {
                 foo: 'foo',
                 0: 'bar',
             });
             expect(path).to.equal('/foo/bar');
         });
-        it('handle query params', function () {
+
+        it('can build query params correctly', function () {
             var path = router.makePath(
-                'unamed_params',
+                'home',
+                {},
                 {
-                    foo: 'foo',
-                    0: 'bar',
-                },
-                {
-                    foo: 'bar',
-                    baz: 'foo',
+                    c: '42',
+                    a: 'bar',
+                    b: ['1', '2', '3'],
                 }
             );
-            expect(path).to.equal('/foo/bar?baz=foo&foo=bar');
+            expect(path).to.equal('/?a=bar&b=1&b=2&b=3&c=42');
+        });
+
+        it('handle query params properly', function () {
+            const originalPath = '/?a=bar&b=42&b=24&c=';
+            const route = router.getRoute(originalPath);
+            const path = router.makePath(route.name, route.params, route.query);
+
+            expect(path).to.equal(originalPath);
         });
+
         it('adds no question mark with empty query', function () {
             var path = router.makePath(
                 'unamed_params',
@@ -545,6 +567,7 @@ describe('Router', function () {
             );
             expect(path).to.equal('/foo/bar');
         });
+
         it('non-existing route', function () {
             var path = router.makePath('article_does_not_exist', {
                 site: 'SITE',
@@ -554,20 +577,24 @@ describe('Router', function () {
             });
             expect(path).to.equal(null);
         });
+
         it('array route', function () {
             var path = router.makePath('array_path', {});
             expect(path).to.equal('/array_path');
         });
+
         it('invalid route', function () {
             var path = router.makePath('invalid_path', {});
             expect(path).to.equal(null);
         });
+
         it('path with some json value and consistency', function () {
             var path = router.makePath('json_value', {
                 json: JSON.stringify({ keyword: 'foo' }),
             });
             expect(path).to.equal(encodingConsistencyPath);
         });
+
         it('array path with different props', function () {
             var pathFoo = router.makePath('array_path_with_different_props', {
                 foo: 'foo',
@@ -664,6 +691,7 @@ describe('Route', function () {
         var homeRoute = router._routes.home;
         expect(homeRoute.match()).to.equal(null, 'empty path returns null');
     });
+
     it('should leave unset optional parameters as undefined', function () {
         var router = new Router(routesObject);
         var article = router._routes.article;