require initialization and only navigate to catchAll if defined (#17)

## Summary

This PR requires consumers to explicitly call `initialize` before consuming the instance. This allows finer control over when the event listeners are added.

We also enable true cleanup by updating `reset` to remove its `onpopstate` event listener _correctly_.

Finally, only navigate to the catchall route if one is defined (prevents runaway navigation if a URL isn't found).

## Test Plan

Added unit tests here for all new behavior and verified it works by `yarn link`ing in the monolith

Author: James Fox

CHANGELOG.md

@@ -0,0 +1,21 @@
+# Changelog
+
+All notable changes to this project will be documented in this file.
+
+The format is based on [Keep a Changelog](http://keepachangelog.com/en/1.0.0/)
+and this project adheres to [Semantic Versioning](http://semver.org/spec/v2.0.0.html).
+
+## [Unreleased]
+
+### Breaking Changes
+
+- Require explicit initialization via `initialize` before using. (See #17)
+
+### Fixes
+
+- Ensure that `reset` cleans up the `onpopstate` listener. (See #17)
+- Only navigate to the `catchAll` path if one has been provided. (See #17)
+
+
+## [1.4.7] - April 5th, 2019
+

package.json

@@ -1,6 +1,6 @@
 {
   "name": "@optimizely/nuclear-router",
-  "version": "1.4.7",
+  "version": "2.0.0",
   "description": "NuclearJS Router",
   "main": "dist/main.js",
   "scripts": {

src/Router.js

@@ -33,12 +33,19 @@ export default class Router {
       base: '',
     }, opts)
 
-    this.setInitialState();
-
     this.onRouteStart = this.opts.onRouteStart;
-    this.onRouteComplete = this.opts.onRouteComplete
+    this.onRouteComplete = this.opts.onRouteComplete;
+    this.__onpopstate = this.__onpopstate.bind(this);
+  }
 
-    WindowEnv.addEventListener('popstate', this.__onpopstate.bind(this))
+  initialize() {
+    this.setInitialState();
+    WindowEnv.addEventListener('popstate', this.__onpopstate);
+  }
+
+  reset() {
+    this.setInitialState();
+    WindowEnv.removeEventListener('popstate', this.__onpopstate);
   }
 
   setInitialState() {
@@ -100,13 +107,10 @@ export default class Router {
     this.__dispatch(canonicalPath, 'replace')
   }
 
-  reset() {
-    this.setInitialState();
-    WindowEnv.removeEventListener('popstate', this.__onpopstate)
-  }
-
   catchall() {
-    WindowEnv.navigate(this.__catchallPath)
+    if (typeof this.__catchallPath === 'string') {
+      WindowEnv.navigate(this.__catchallPath);
+    }
   }
 
   /**

test/Router.spec.js

@@ -25,29 +25,78 @@ describe('Router', () => {
     sinon.stub(HistoryEnv, 'pushState')
     sinon.stub(HistoryEnv, 'replaceState')
     sinon.stub(DocumentEnv, 'getTitle').returns(pageTitle)
+    sinon.stub(WindowEnv, 'addEventListener')
+    sinon.stub(WindowEnv, 'removeEventListener')
   })
 
   afterEach(() => {
     HistoryEnv.pushState.restore()
     HistoryEnv.replaceState.restore()
     DocumentEnv.getTitle.restore()
+    WindowEnv.addEventListener.restore()
+    WindowEnv.removeEventListener.restore()
   })
 
   describe('construction', () => {
-    beforeEach(() => {
-      sinon.stub(WindowEnv, 'addEventListener')
-    })
-
-    afterEach(() => {
-      WindowEnv.addEventListener.restore()
-    })
-
     it('should create a Router instance', () => {
       let router = new Router
       expect(router instanceof Router).toBe(true)
     })
   })
 
+  describe('initialization and reset', function() {
+    let emptyRouter;
+    beforeEach(function() {
+      emptyRouter = new Router({});
+    });
+
+    context('initialize', function() {
+      it('should not define state before initialization', function() {
+        expect(emptyRouter.__fromPath).toBe(undefined);
+        expect(emptyRouter.__routes).toBe(undefined);
+        expect(emptyRouter.__currentCanonicalPath).toBe(undefined);
+        sinon.assert.notCalled(WindowEnv.addEventListener);
+      });
+
+      it('should set initial state and add popstate listener upon initialization', function() {
+        emptyRouter.initialize();
+        expect(emptyRouter.__fromPath).toBe(null);
+        expect(emptyRouter.__routes.length).toBe(0);
+        expect(emptyRouter.__currentCanonicalPath).toBe(null);
+        sinon.assert.calledOnce(WindowEnv.addEventListener);
+        sinon.assert.calledWith(WindowEnv.addEventListener, 'popstate');
+      });
+    })
+
+    context('reset', function() {
+      beforeEach(function() {
+        emptyRouter.initialize();
+        emptyRouter.registerRoutes([
+          {
+            match: '/the_route',
+            handle: [
+              (ctx, next) => {
+                next()
+              },
+            ],
+          },
+        ]);
+        emptyRouter.go('/the_route');
+        return getMacroTaskResolvedPromise();
+      })
+      it('should reset initial state and remove popstate listener', function() {
+        expect(emptyRouter.__routes.length).toBe(1);
+        sinon.assert.notCalled(WindowEnv.removeEventListener);
+        emptyRouter.reset();
+        expect(emptyRouter.__fromPath).toBe(null);
+        expect(emptyRouter.__routes.length).toBe(0);
+        expect(emptyRouter.__currentCanonicalPath).toBe(null);
+        sinon.assert.calledOnce(WindowEnv.removeEventListener);
+        sinon.assert.calledWith(WindowEnv.removeEventListener, 'popstate');
+      });
+    })
+  });
+
   describe('navigation', () => {
     let router
     let spy1, spy2, spy3
@@ -65,6 +114,7 @@ describe('Router', () => {
         },
         onRouteStart: onRouteStartSpy,
       })
+      router.initialize();
 
       spy1 = sinon.spy()
       spy2 = sinon.spy()
@@ -197,6 +247,16 @@ describe('Router', () => {
             sinon.assert.calledWithExactly(WindowEnv.navigate, '/404');
           })
       });
+
+      it('should not window navigate if the catchall route is not set', () => {
+        router.__catchallPath = null;
+        router.go('/abcdefg');
+
+        return getMacroTaskResolvedPromise()
+          .then(() => {
+            sinon.assert.notCalled(WindowEnv.navigate);
+          })
+      });
     });
 
     describe('when a single route matches', function() {