refactor(RouteTraverser): Expand traverser to also track query params…

… and location changes (#85)

Traverser now accepts a LocationChange instead of a pathname. Returned Match now includes the LocationChange and the query params. RouterInstruction is no longer responsible for parsing query params. Guards now receive query params and the location change as a result of this refactoring.

BREAKING CHANGE:

  TraversalCandidate interface has renamed `params` to `routeParams` and now includes queryParams and locationChange.

  Before:

  ```ts
  const { params, isTerminal, route } = traversalCandidate;
  ```

  After:

  ```ts
  const { routeParams, queryParams, isTerminal, route, locationChange } = traversalCandidate;
  ```

docs/overview/guards.md

@@ -19,15 +19,28 @@ import 'rxjs/add/operator/map';
 import { Injectable } from '@angular/core';
 import { Http } from '@angular/http';
 import { Observable } from 'rxjs/Observable';
-import { Guard, Route, TraversalCandidate } from '@ngrx/router';
+import { Guard, Route, TraversalCandidate, LocationChange } from '@ngrx/router';
 
 @Injectable()
 class AuthGuard implements Guard {
   constructor(private _http: Http) { }
-  // Guards are provided with a traversal candidate object which contains a
-  // snapshot of the route params parsed so far, the parsed query params,
-  // the route being evaluated, and the location change that caused traversal.
-  protectRoute(candidate: TraversalCandidate) {
+
+  protectRoute(candidate: TraversalCandidate) {    
+    // `route` is the current route being evaluated
+    const route: Route = candidate.route;
+
+    // `locationChange` includes the full path and type of change that caused traversal
+    const locationChange: LocationChange = candidate.locationChange;
+
+    // `queryParams` are the parsed query parameters
+    const queryParams: any = candidate.queryParams;
+
+    // `routeParams` is a snapshot of the route parameters discovered so far
+    const routeParams: any = candidate.routeParams;
+
+    // `isTerminal` indicates that the candidate route is going to be the last route traversed
+    const isTerminal: boolean = candidate.isTerminal;
+
     return this._http.get('/auth/check')
       // If request succeeds, return true
       .map(() => true)

lib/index.ts

@@ -53,13 +53,13 @@ export function provideRouter(routes: Routes, locationStrategy: Type = PathLocat
 }
 
 
-export { Guard, provideGuard } from './guard';
+export { Guard } from './guard';
 export { LocationChange, Router } from './router';
 export { RouteParams, QueryParams } from './params';
-export { ROUTER_HOOKS, INSTRUCTION_HOOKS, RouterInstruction, NextInstruction } from './router-instruction';
+export { ROUTER_HOOKS, INSTRUCTION_HOOKS, RouterInstruction } from './router-instruction';
 export { PRE_RENDER_HOOKS, POST_RENDER_HOOKS, RenderInstruction } from './component-renderer';
 export { Routes, Route, IndexRoute } from './route';
-export { TRAVERSAL_HOOKS, TraversalCandidate } from './route-traverser';
+export { TRAVERSAL_HOOKS, TraversalCandidate, Match } from './route-traverser';
 export { LinkTo } from './link-to';
 export { LinkActive, LinkActiveOptions } from './link-active';
 export { RouteView } from './route-view';

lib/redirect.ts

@@ -8,19 +8,20 @@ import { Provider, Injectable } from '@angular/core';
 
 import { Router } from './router';
 import { Routes, Route } from './route';
-import { INSTRUCTION_HOOKS, NextInstruction } from './router-instruction';
+import { INSTRUCTION_HOOKS } from './router-instruction';
+import { Match } from './route-traverser';
 import { formatPattern } from './match-pattern';
 import { Hook } from './hooks';
 
 
 @Injectable()
-export class RedirectHook implements Hook<NextInstruction> {
+export class RedirectHook implements Hook<Match> {
   constructor(private router: Router) { }
 
-  apply(next$: Observable<NextInstruction>): Observable<NextInstruction> {
+  apply(next$: Observable<Match>): Observable<Match> {
     return next$
       .filter(next => {
-        const last = next.routeConfigs[next.routeConfigs.length - 1];
+        const last = next.routes[next.routes.length - 1];
 
         if (last.redirectTo) {
           this._handleRedirect(last, next);
@@ -31,7 +32,7 @@ export class RedirectHook implements Hook<NextInstruction> {
       });
   }
 
-  private _handleRedirect(route: Route, next: NextInstruction) {
+  private _handleRedirect(route: Route, next: Match) {
     const { routeParams, queryParams } = next;
 
     let pathname;
@@ -40,8 +41,8 @@ export class RedirectHook implements Hook<NextInstruction> {
       pathname = formatPattern(route.redirectTo, routeParams);
     }
     else {
-      const routeIndex = next.routeConfigs.indexOf(route);
-      const parentPattern = this._getRoutePattern(next.routeConfigs, routeIndex - 1);
+      const routeIndex = next.routes.indexOf(route);
+      const parentPattern = this._getRoutePattern(next.routes, routeIndex - 1);
       const pattern = parentPattern.replace(/\/*$/, '/') + route.redirectTo;
       pathname = formatPattern(pattern, routeParams);
     }

lib/route-traverser.ts

@@ -13,24 +13,30 @@ import 'rxjs/add/operator/concatMap';
 import 'rxjs/add/operator/take';
 import { Observable } from 'rxjs/Observable';
 import { OpaqueToken, Provider, Inject, Injectable, Optional } from '@angular/core';
+import * as queryString from 'query-string';
 
 import { ResourceLoader, Async } from './resource-loader';
 import { matchPattern, makeParams } from './match-pattern';
 import { Route, IndexRoute, Routes, ROUTES } from './route';
 import { Hook, composeHooks } from './hooks';
+import { LocationChange } from './router';
 
 export const TRAVERSAL_HOOKS = new OpaqueToken(
   '@ngrx/router Traversal Hooks'
 );
 
 export interface Match {
   routes: Routes;
-  params: any;
+  routeParams: any;
+  queryParams: any;
+  locationChange: LocationChange;
 };
 
 export interface TraversalCandidate {
   route: Route;
-  params: any;
+  routeParams: any;
+  queryParams: any;
+  locationChange: LocationChange;
   isTerminal: boolean;
 }
 
@@ -51,24 +57,30 @@ export class RouteTraverser {
   * - routes       An array of routes that matched, in hierarchical order
   * - params       An object of URL parameters
   */
-  find(pathname: string) {
-    return this._matchRoutes(this._routes, pathname);
+  find(change: LocationChange) {
+    const [ pathname, query ] = change.path.split('?');
+    const queryParams = queryString.parse(query);
+    return this._matchRoutes(queryParams, change, pathname);
   }
 
   private _matchRoutes(
-    routes: Routes,
+    queryParams: any,
+    locationChange: LocationChange,
     pathname: string,
     remainingPathname = pathname,
-    paramNames = [],
-    paramValues = []
+    routes: Routes = this._routes,
+    routeParamNames = [],
+    routeParamValues = []
   ): Observable<Match> {
     return Observable.from(routes)
       .concatMap(route => this._matchRouteDeep(
         route,
+        queryParams,
+        locationChange,
         pathname,
         remainingPathname,
-        paramNames,
-        paramValues
+        routeParamNames,
+        routeParamValues
       ))
       .catch(error => {
         console.error('Error During Traversal', error);
@@ -80,6 +92,8 @@ export class RouteTraverser {
 
   private _matchRouteDeep(
     route: Route,
+    queryParams: any,
+    locationChange: LocationChange,
     pathname: string,
     remainingPathname: string,
     paramNames: string[],
@@ -105,17 +119,21 @@ export class RouteTraverser {
       .map<TraversalCandidate>(() => {
         return {
           route,
-          params: makeParams(paramNames, paramValues),
+          queryParams,
+          locationChange,
+          routeParams: makeParams(paramNames, paramValues),
           isTerminal: remainingPathname === '' && !!route.path
         };
       })
       .let<TraversalCandidate>(composeHooks(this._hooks))
       .filter(({ route }) => !!route)
-      .mergeMap(({ route, params, isTerminal }) => {
+      .mergeMap(({ route, routeParams, queryParams, isTerminal }) => {
         if ( isTerminal ) {
           const match: Match = {
             routes: [ route ],
-            params
+            routeParams,
+            queryParams,
+            locationChange
           };
 
           return Observable.of(route)
@@ -132,9 +150,11 @@ export class RouteTraverser {
         return Observable.of(route)
           .mergeMap(route => this._loadChildRoutes(route))
           .mergeMap<Match>(childRoutes => this._matchRoutes(
-            childRoutes,
+            queryParams,
+            locationChange,
             pathname,
             remainingPathname,
+            childRoutes,
             paramNames,
             paramValues
           ))

lib/route-view.ts

@@ -24,8 +24,9 @@ import {
 } from '@angular/core';
 
 import { Route, getNamedComponents } from './route';
-import { RouterInstruction, NextInstruction } from './router-instruction';
+import { RouterInstruction } from './router-instruction';
 import { ComponentRenderer } from './component-renderer';
+import { Match } from './route-traverser';
 
 @Component({
   selector: 'route-view',
@@ -36,10 +37,10 @@ export class RouteView implements OnDestroy, OnInit {
   private _prev: ComponentRef;
   private _sub: any;
   private _routerInstructionProvider = new Provider(RouterInstruction, {
-    useValue: this._routerInstruction$.map<NextInstruction>(set => {
+    useValue: this._routerInstruction$.map<Match>(set => {
       return {
         locationChange: set.locationChange,
-        routeConfigs: [ ...set.routeConfigs ].slice(1),
+        routes: [ ...set.routes ].slice(1),
         routeParams: set.routeParams,
         queryParams: set.queryParams
       };
@@ -58,7 +59,7 @@ export class RouteView implements OnDestroy, OnInit {
   ngOnInit() {
     this._sub = this._routerInstruction$
       .map(set => {
-        const route = set.routeConfigs[0];
+        const route = set.routes[0];
         const components = getNamedComponents(route, this._name);
 
         return { route, components };

lib/router-instruction.ts

@@ -22,46 +22,27 @@ import { Hook, composeHooks } from './hooks';
 export const ROUTER_HOOKS = new OpaqueToken('@ngrx/router Router Hooks');
 export const INSTRUCTION_HOOKS = new OpaqueToken('@ngrx/router Instruction Hooks');
 
-export interface NextInstruction {
-  routeConfigs: Routes;
-  routeParams: any;
-  queryParams: any;
-  locationChange: LocationChange;
-}
-
 
-export abstract class RouterInstruction extends Observable<NextInstruction> { }
+export abstract class RouterInstruction extends Observable<Match> { }
 
 @Injectable()
 export class RouterInstructionFactory {
   constructor(
     private _router$: Router,
     private _traverser: RouteTraverser,
-    private _zoneOperator: ZoneOperator<NextInstruction>,
+    private _zoneOperator: ZoneOperator<Match>,
     @Optional() @Inject(ROUTER_HOOKS)
       private _routerHooks: Hook<LocationChange>[] = [],
     @Optional() @Inject(INSTRUCTION_HOOKS)
-      private _instructionHooks: Hook<NextInstruction>[] = []
+      private _instructionHooks: Hook<Match>[] = []
   ) { }
 
   create(): RouterInstruction {
     return this._router$
       .observeOn(asap)
       .distinctUntilChanged((prev, next) => prev.path === next.path)
       .let(composeHooks(this._routerHooks))
-      .switchMap(change => {
-        const [ pathname, queryString ] = change.path.split('?');
-
-        return this._traverser.find(pathname)
-          .map<NextInstruction>(set => {
-            return {
-              locationChange: change,
-              routeConfigs: set.routes,
-              queryParams: parseQueryString(queryString),
-              routeParams: set.params
-            };
-          });
-      })
+      .switchMap(change => this._traverser.find(change))
       .filter(match => !!match)
       .let(composeHooks(this._instructionHooks))
       .lift(this._zoneOperator)

spec/guard.spec.ts

@@ -19,8 +19,14 @@ describe('Guard Middleware', function() {
     protectRoute = () => Observable.of(false);
   }
 
-  function route(route: Route, params = {}, isTerminal = false) {
-    return Observable.of({ route, params, isTerminal });
+  function route(
+    route: Route,
+    routeParams = {},
+    queryParams = {},
+    isTerminal = false,
+    locationChange = { type: 'push', path: '/' }
+  ): Observable<TraversalCandidate> {
+    return Observable.of({ route, routeParams, queryParams, isTerminal, locationChange });
   }
 
   beforeEach(function() {
@@ -56,19 +62,26 @@ describe('Guard Middleware', function() {
     expect(injector.resolveAndInstantiate).toHaveBeenCalledWith(PassGuard);
   });
 
-  // Intentionally commenting this out because a future PR will refactor
-  // traversal candidate and will pass that to the guards instead
-  xit('should provide guards with the TraversalCandidate', function() {
-    // const testGuard = { run: () => Observable.of(true) };
-    // spyOn(testGuard, 'run').and.callThrough();
-    // const guard = provideGuard(() => testGuard.run);
-    // const nextRoute = { guards: [ guard ] };
-    // const params = { abc: 123 };
-    // const isTerminal = true;
-    //
-    // route(nextRoute, params, isTerminal).let(t => guardHook.apply(t)).subscribe();
-    //
-    // expect(testGuard.run).toHaveBeenCalledWith(params, nextRoute, isTerminal);
+
+  it('should provide guards with the TraversalCandidate', function() {
+    const spy = jasmine.createSpy('protectRoute').and.returnValue(Observable.of(true));
+    class MockGuard {
+      protectRoute = spy;
+    }
+
+    const traversalCandidate: TraversalCandidate = {
+      route: { path: '/', guards: [ MockGuard ] },
+      queryParams: {},
+      routeParams: {},
+      locationChange: { type: 'push', path: '/' },
+      isTerminal: true
+    };
+
+    const stream$ = Observable.of(traversalCandidate);
+
+    stream$.let(t => guardHook.apply(t)).subscribe();
+
+    expect(spy).toHaveBeenCalledWith(traversalCandidate);
   });
 
   it('should return true if all of the guards return true', function(done) {

spec/params.spec.ts

@@ -1,18 +1,19 @@
 import { Subject } from 'rxjs/Subject';
 import { ReflectiveInjector, provide } from '@angular/core';
 
-import { NextInstruction, RouterInstruction } from '../lib/router-instruction';
+import { RouterInstruction } from '../lib/router-instruction';
+import { Match } from '../lib/route-traverser';
 import { RouteParams, QueryParams, PARAMS_PROVIDERS } from '../lib/params';
 
 
 describe('Params Services', function() {
-  let routerInstruction$: Subject<NextInstruction>;
+  let routerInstruction$: Subject<Match>;
   let routeParams$: RouteParams;
   let queryParams$: QueryParams;
 
   function nextInstruction(routeParams, queryParams) {
     routerInstruction$.next({
-      routeConfigs: [],
+      routes: [],
       routeParams,
       queryParams,
       locationChange: {
@@ -23,7 +24,7 @@ describe('Params Services', function() {
   }
 
   beforeEach(function() {
-    routerInstruction$ = new Subject<NextInstruction>();
+    routerInstruction$ = new Subject<Match>();
     const injector = ReflectiveInjector.resolveAndCreate([
       PARAMS_PROVIDERS,
       provide(RouterInstruction, { useValue: routerInstruction$ })