fix(browser-history): decode fragment, add tests

aurelia · Apr 21, 2019 · 957532d · davismj · May 7, 2019 · bigopon
1 parent 4cf18cf
commit 957532d
Show file tree

Hide file tree

Showing 2 changed files with 38 additions and 6 deletions.
diff --git a/src/browser-history.ts b/src/browser-history.ts
@@ -12,8 +12,6 @@ export class BrowserHistory extends History {
   /**@internal */
   _isActive: boolean;
 
-  /**@internal*/
-  _checkUrlCallback: any;
   /**@internal*/
   location: Location;
   /**@internal*/
@@ -39,7 +37,7 @@ export class BrowserHistory extends History {
     super();
 
     this._isActive = false;
-    this._checkUrlCallback = this._checkUrl.bind(this);
+    this._checkUrl = this._checkUrl.bind(this);
 
     this.location = PLATFORM.location;
     this.history = PLATFORM.history;
@@ -76,7 +74,7 @@ export class BrowserHistory extends History {
       eventName = 'hashchange';
     }
 
-    PLATFORM.addEventListener(eventName, this._checkUrlCallback);
+    PLATFORM.addEventListener(eventName, this._checkUrl);
 
     // Determine if we need to change the base url, for a pushState link
     // opened by a non-pushState browser.
@@ -116,7 +114,7 @@ export class BrowserHistory extends History {
    * Deactivates the history object.
    */
   deactivate(): void {
-    const handler = this._checkUrlCallback;
+    const handler = this._checkUrl;
     PLATFORM.removeEventListener('popstate', handler);
     PLATFORM.removeEventListener('hashchange', handler);
     this._isActive = false;
@@ -157,6 +155,8 @@ export class BrowserHistory extends History {
       return false;
     }
 
+    // caching fragment value to prevent triggering load same URL twice
+    // as this could potentially trigger hashchange or pushstate
     this.fragment = fragment;
 
     let url = this.root + fragment;
@@ -271,7 +271,9 @@ export class BrowserHistory extends History {
       }
     }
 
-    return '/' + fragment.replace(routeStripper, '');
+    // without decoding the fragment
+    // _loadUrl will be trigger twice if there are special character in the URL
+    return decodeURIComponent('/' + fragment.replace(routeStripper, ''));
   }
 
   /**
@@ -281,6 +283,8 @@ export class BrowserHistory extends History {
    */
   _checkUrl(): void {
     let current = this._getFragment('');
+    // a guard to prevent triggering load same URL twice
+    // typically happens when calling navigate from router
     if (current !== this.fragment) {
       this._loadUrl('');
     }

diff --git a/test/history.spec.ts b/test/history.spec.ts
@@ -34,6 +34,34 @@ describe('browser history', () => {
       expect(bh._getFragment('#/admin/user/123   ')).toBe(expected);
       expect(bh._getFragment('#///admin/user/123')).toBe(expected);
     });
+
+    const testCases = [
+      [encodeURIComponent('admin user 123'), '/admin user 123'],
+      [encodeURIComponent('ad"min user'), '/ad"min user'],
+      [encodeURIComponent('ad{min  '), '/ad{min  '],
+      ...['@', '!', '$', '%', '&', '^', '*', '(', ')', '[', ']', '\\', '|', '{', '}', ':', ';', '\'', ' ', '  ', '   ', '"', '<', '>', '~', '`',
+        ...[
+          '( ͡° ͜ʖ ͡°)',
+          'ʕ•ᴥ•ʔ',
+          '༼ つ ◕_◕ ༽つ',
+          '(ಥ﹏ಥ)',
+          'ლ(ಠ益ಠლ)',
+          '◉_◉',
+          'ಠ⌣ಠ',
+          'ಠ~ಠ'
+        ]
+      ]
+        .map(specialChar => {
+          return [encodeURIComponent(`ad${specialChar}min/ad${specialChar}min`), `/ad${specialChar}min/ad${specialChar}min`];
+        })
+    ];
+
+    testCases.forEach(([input, output]) => {
+      it(`should decode fragment "${input}" correctly`, async () => {
+        let bh = new BrowserHistory(null);
+        expect(bh._getFragment(input)).toBe(output);
+      });
+    });
   });
 
   describe('getAbsoluteRoot', () => {