3
\$\begingroup\$

WARNING: Please see @MgSam's answer for a fatal bug in my original code. He has a revised version that fixes the bug.


I have made a route interceptor service that has an API to hook into every event that the router broadcasts, and pass in iteratee, a function that will be invoked on each event and takes in the route as an argument.

Also, the route that is passed in is a modified copy of the original. The difference is that any BehaviorSubject observables (such as the route data) will be accessible directly as values instead of having the need to subscribe to them.

Here is the implementation of the service:

import { Component, OnInit } from '@angular/core';
import { Title } from '@angular/platform-browser';

import { RouteInterceptorService } from './shared';

@Component({
    selector: 'app-root',
    templateUrl: './app.component.html'
})
export class AppComponent implements OnInit {
    constructor(
        private routeInterceptorService: RouteInterceptorService,
        private titleService: Title
    ) {}

    ngOnInit(): void {
        this.routeInterceptorService.getRouteOnNavigationEnd(route => {
            this.titleService.setTitle(route.data.title);
        });
    }
}

As you can see, I'm listening to the NavigationEnd event and updating the browser title based on the title param in defined on the respective route.

Here is the RouteInterceptorService:

import { Injectable } from '@angular/core';
import {
    ActivatedRoute,
    Router,
    Event,
    NavigationStart,
    NavigationEnd,
    NavigationCancel,
    NavigationError,
    RoutesRecognized,
    RouteConfigLoadStart,
    RouteConfigLoadEnd
} from '@angular/router';

import { Observable } from 'rxjs';
import 'rxjs/add/operator/map';
import 'rxjs/add/operator/filter';
import 'rxjs/add/operator/mergeMap';

enum RouteInterceptorEvents {
    NAVIGATION_START,
    NAVIGATION_END,
    NAVIGATION_CANCEL,
    NAVIGATION_ERROR,
    CONFIG_LOAD_START,
    CONFIG_LOAD_END,
    ROUTE_RECOGNIZED
}

@Injectable()
export class RouteInterceptorService {
    events: Map<number, Observable<Event>> = new Map();

    constructor(
        private activatedRoute: ActivatedRoute,
        private router: Router
    ) {
        this.populateEventMap();
    }

    getRouteOnNavigationStart(iteratee: Function) {
        this.getRouteOn(RouteInterceptorEvents.NAVIGATION_START, iteratee);
    }

    getRouteOnNavigationEnd(iteratee: Function) {
        this.getRouteOn(RouteInterceptorEvents.NAVIGATION_END, iteratee);
    }

    getRouteOnNavigationCancel(iteratee: Function) {
        this.getRouteOn(RouteInterceptorEvents.NAVIGATION_CANCEL, iteratee);
    }

    getRouteOnNavigationError(iteratee: Function) {
        this.getRouteOn(RouteInterceptorEvents.NAVIGATION_ERROR, iteratee);
    }

    getRouteOnConfigLoadStart(iteratee: Function) {
        this.getRouteOn(RouteInterceptorEvents.CONFIG_LOAD_START, iteratee);
    }

    getRouteOnConfigLoadEnd(iteratee: Function) {
        this.getRouteOn(RouteInterceptorEvents.CONFIG_LOAD_END, iteratee);
    }

    getRouteOnRouteRecognized(iteratee: Function) {
        this.getRouteOn(RouteInterceptorEvents.ROUTE_RECOGNIZED, iteratee);
    }

    private getRouteOn(eventType: number, iteratee: Function) {
        this.events[eventType]
            // By returning a new Object (this.activatedRoute) into the stream, we
            // essentially swap what we're observing.
            // At this point we only run .map() if the filtered event (this.events[eventType])
            // successfully returns the event, meaning the event of 'eventType' has been triggered.
            .map(() => this.activatedRoute)
            // Traverse the state tree to find the last activated route, and then
            // return it to the stream. Doing this allow sus to dive into the 'children'
            // property of the routes config to fetch the corresponding page title(s).
            .map(route => {
                while (route.firstChild) route = route.firstChild;
                return route;
            })
            // Filter un-named (primary) router outlets only.
            .filter(route => route.outlet === 'primary')
            // Get the value of all the BehaviorSubject observables in the route so that
            // the values can be directly accessed from the route like 'route.data.foo'
            // instead of having to subscribe to the observable at the 'iteratee' callback
            // level and receive the data that way.
            .map(route => {
                let keys = Object.keys(route);
                let len = keys.length;

                for (let i = 0; i < len; i++) {
                    if (route[keys[i]].getValue) {
                        route[keys[i]] = route[keys[i]].getValue();
                    }
                }

                return route;
            })
            .subscribe(route => iteratee(route));
    }

    private populateEventMap() {
        this.events[RouteInterceptorEvents.NAVIGATION_START] = this.router.events.filter(e => e instanceof NavigationStart);
        this.events[RouteInterceptorEvents.NAVIGATION_END] =  this.router.events.filter(e => e instanceof NavigationEnd);
        this.events[RouteInterceptorEvents.NAVIGATION_CANCEL] =  this.router.events.filter(e => e instanceof NavigationCancel);
        this.events[RouteInterceptorEvents.NAVIGATION_ERROR] =  this.router.events.filter(e => e instanceof NavigationError);
        this.events[RouteInterceptorEvents.CONFIG_LOAD_START] =  this.router.events.filter(e => e instanceof RouteConfigLoadStart);
        this.events[RouteInterceptorEvents.CONFIG_LOAD_END] =  this.router.events.filter(e => e instanceof RouteConfigLoadEnd);
        this.events[RouteInterceptorEvents.ROUTE_RECOGNIZED] =  this.router.events.filter(e => e instanceof RoutesRecognized);
    }
}

Are there any improvements you think that can/should be made to this, or any corner cases that I may not be thinking of?

As is, it works just fine.

\$\endgroup\$
  • \$\begingroup\$ This whole concept of messing with router events scares me! Does this still work? \$\endgroup\$ – Simon_Weaver Mar 2 at 8:42
2
\$\begingroup\$

For me it's a situation where it's really hard to make a suggestion. To me the code looks pretty clean. I can only make a few points on style, which is probably subjective to a certain degree.

Comments In The Code

I am generally not a fan of comments -- developers can live with "just code". Maintaining comments is burdensome. I recommend you find a way to get rid of them.

Refactor -> Make Comments Useless -> Remove 'Em

this.events[eventType]
  // By returning a new Object (this.activatedRoute) into the stream, we
  // essentially swap what we're observing.
  // ...
  .map(() => this.activatedRoute)
  .map(route => ...)

If the reader has to deal with RxJs, he/she must know what Observable.map() does. You may still express the intent, though. To express the intent, you may extract code into a private method. This will, of course, change a bit the way we chain, but the code is still readable:

this.swapEventToObserveActivatedRoute(eventType) // <--- PLEASE FIND MORE EXPLANATORY NAME
  .map(route => {...})

Make Them (Devs) Read The Code [Maybe?]

The following comment is not helping me at all -- I see what the code does.

// Traverse the state tree to find the last activated route, and then...

Some Comments Are Fine

While the following comment explains both the intent AND the implementation, it is better candidate for staying the way it is. The reason is Angular developers don't have to deal with these aspects on daily basis and a bit of context may be helpful.

// Get the value of all the BehaviorSubject observables in the route so that

I'd still try to extract this into a separate method and give it a name.

const filteredRoute = this
  .swapEventToObserveActivatedRoute(eventType)
  .map(...)
  .filter(...);

filteredRoute
  .map(...)
  .subscribe(...);

Why Deal With Indices In Loops?

The last map function can be rewritten the way that does not require manual counters/indices manipulation. This is very typical for functional programming. This approach helps making sure that there's no chance for the infamous OBOE.

.map(route => {
    Object.keys(route).forEach(routeKey => {
        if (route[routeKey].getValue)
            route[routeKey] = route[routeKey].getValue();
    });
    return route;
})

On Naming + On TypeScript And Types

I strongly recommend making the function result type explicit.

When I see the get* method, I expect it to return something. However, the return type is void. That causes immediate confusion: is it a misleading name or is the code missing a return statement?

getRouteOnRouteRecognized(iteratee: Function) {              // <-- THIS GUY
    this.getRouteOn(RouteInterceptorEvents.ROUTE_RECOGNIZED, iteratee);
}
private getRouteOn(eventType: number, iteratee: Function) {  // <-- THIS GUY TOO
\$\endgroup\$
  • 2
    \$\begingroup\$ Lots of good advice, thanks! I renamed the getRouteOn... methods to onNavigationEnd, etc. Much easier to read that way and doesn't falsely imply something is going to be returned. Also removed a bunch of the comments or converted them to compact one liners. \$\endgroup\$ – Lansana Apr 26 '17 at 19:27
  • 2
    \$\begingroup\$ @Lansana good job! The comment thing is pretty common for the passionate devs -- we like telling the stories. Often, we can make the code to be a story, but it requires attention and discipline. As I said, my recommendations are stylistic in nature, so I assume nobody in your team will say you're doing something "wrong" \$\endgroup\$ – Igor Soloydenko Apr 26 '17 at 19:30
  • \$\begingroup\$ Totally disagree about the comments suggestions. Comments are great. Yes, they can become outdated. But that is the exception- most of the time code isn't being constantly re-written. If anything, I think this should have JSDocs on all the public API surface area. \$\endgroup\$ – MgSam Feb 14 '18 at 20:56
  • \$\begingroup\$ @MgSam all the comments in the original code add zero value. Almost always, comments are written by someone who was lazy to write self-documenting, clean, readable code. Documenting a hack around a known hard-to-resolve framework issue may be an exception. Documenting the API surface area is a completely different question, unrelated to this post. Comments are rarely great. They are crap which stales, misleads, requires maintenance. code isn't being constantly re-written -- yeah?? (No, not really). \$\endgroup\$ – Igor Soloydenko Feb 14 '18 at 21:15
  • \$\begingroup\$ @IgorSoloydenko Well, you are provably wrong about this; I found this post because I needed code that did what the author intended, implemented it, found the code has a bug, needed to fix the bug, and found that the comments were helpful to me figuring out what the code actually did. If anything, it needs more of them. It seems pretty presumptuous of you to say "comments are bad", when 50+ years of computer science best practices say otherwise. \$\endgroup\$ – MgSam Feb 15 '18 at 17:44
4
\$\begingroup\$

I found this post because I needed something that did what you implemented. I tried to use the code and ran into some nasty routing bugs whereby Angular was experiencing internal errors. The cause is this bit:

        // Get the value of all the BehaviorSubject observables in the route so that
        // the values can be directly accessed from the route like 'route.data.foo'
        // instead of having to subscribe to the observable at the 'iteratee' callback
        // level and receive the data that way.
        .map(route => {
            let keys = Object.keys(route);
            let len = keys.length;

            for (let i = 0; i < len; i++) {
                if (route[keys[i]].getValue) {
                    route[keys[i]] = route[keys[i]].getValue();
                }
            }

            return route;
        })

This is extremely dangerous. You are modifying an object that you do not own, an object which Angular relies upon to do its routing. Angular can change how they use that object inside the routing module at any time. Changing the properties here is asking for trouble.

In general, you should never modify an object that you didn't create yourself, regardless of which framework you're using.

As an aside, I think the comments are great and, in addition, you should add JSDoc comments to the public surface area of this class.

In case anyone in the future needs something they can just copy and paste- here is a version without the offending code:

import { Injectable } from '@angular/core';
import {
    ActivatedRoute,
    Router,
    Event,
    NavigationStart,
    NavigationEnd,
    NavigationCancel,
    NavigationError,
    RoutesRecognized,
    RouteConfigLoadStart,
    RouteConfigLoadEnd,
} from '@angular/router';

import { Observable } from 'rxjs';
import 'rxjs/add/operator/map';
import 'rxjs/add/operator/filter';
import 'rxjs/add/operator/mergeMap';


/**
 * This service intercepts all routing requests going through Angular.
 * Source: https://codereview.stackexchange.com/questions/161783/angular2-route-interceptor
 */
@Injectable()
export class RouteInterceptor {
    private events: Map<number, Observable<Event>> = new Map();

    constructor(
        private activatedRoute: ActivatedRoute,
        private router: Router
    ) {
        this.populateEventMap();
    }

    /**
     * Signs up a callback for when navigation starts.
     * @param callback The callback function to invoke.
     */
    onNavigationStart(callback: (router: ActivatedRoute) => void) {
        this.getRouteOn(RouteInterceptorEvents.NavigationStart, callback);
    }

    /**
     * Signs up a callback for when navigation ends.
     * @param callback The callback function to invoke.
     */
    onNavigationEnd(callback: (router: ActivatedRoute) => void) {
        this.getRouteOn(RouteInterceptorEvents.NavigationEnd, callback);
    }

    /**
     * Signs up a callback for when navigation is cancelled.
     * @param callback The callback function to invoke.
     */
    onNavigationCancel(callback: (router: ActivatedRoute) => void) {
        this.getRouteOn(RouteInterceptorEvents.NavigationCancel, callback);
    }

    /**
     * Signs up a callback for when navigation errors.
     * @param callback The callback function to invoke.
     */
    onNavigationError(callback: (router: ActivatedRoute) => void) {
        this.getRouteOn(RouteInterceptorEvents.NavigationError, callback);
    }

    /**
     * Signs up a callback for when configuration load starts.
     * @param callback The callback function to invoke.
     */
    onConfigLoadStart(callback: (router: ActivatedRoute) => void) {
        this.getRouteOn(RouteInterceptorEvents.ConfigLoadStart, callback);
    }

    /**
     * Signs up a callback for when configuration load ends.
     * @param callback The callback function to invoke.
     */
    onConfigLoadEnd(callback: (router: ActivatedRoute) => void) {
        this.getRouteOn(RouteInterceptorEvents.ConfigLoadEnd, callback);
    }

    /**
     * Signs up a callback for when the route is recongized.
     * @param callback The callback function to invoke.
     */
    onRouteRecognized(callback: (router: ActivatedRoute) => void) {
        this.getRouteOn(RouteInterceptorEvents.RouteRecognized, callback);
    }

    private getRouteOn(eventType: number, callback: (router: ActivatedRoute) => void) {
        this.events[eventType]
            // By returning a new Object (this.activatedRoute) into the stream, we
            // essentially swap what we're observing.
            // At this point we only run .map() if the filtered event (this.events[eventType])
            // successfully returns the event, meaning the event of 'eventType' has been triggered.
            .map(() => this.activatedRoute)
            // Traverse the state tree to find the last activated route, and then
            // return it to the stream. Doing this allows us to dive into the 'children'
            // property of the routes config to fetch the corresponding page title(s).
            .map(route => {
                while (route.firstChild) route = route.firstChild;
                return route;
            })
            // Filter un-named (primary) router outlets only.
            .filter(route => route.outlet === 'primary')
            .subscribe(route => callback(route));
    }

    private populateEventMap() {
        this.events[RouteInterceptorEvents.NavigationStart] = this.router.events.filter(e => e instanceof NavigationStart);
        this.events[RouteInterceptorEvents.NavigationEnd] = this.router.events.filter(e => e instanceof NavigationEnd);
        this.events[RouteInterceptorEvents.NavigationCancel] = this.router.events.filter(e => e instanceof NavigationCancel);
        this.events[RouteInterceptorEvents.NavigationError] = this.router.events.filter(e => e instanceof NavigationError);
        this.events[RouteInterceptorEvents.ConfigLoadStart] = this.router.events.filter(e => e instanceof RouteConfigLoadStart);
        this.events[RouteInterceptorEvents.ConfigLoadEnd] = this.router.events.filter(e => e instanceof RouteConfigLoadEnd);
        this.events[RouteInterceptorEvents.RouteRecognized] = this.router.events.filter(e => e instanceof RoutesRecognized);
    }
}

enum RouteInterceptorEvents {
    NavigationStart,
    NavigationEnd,
    NavigationCancel,
    NavigationError,
    ConfigLoadStart,
    ConfigLoadEnd,
    RouteRecognized
}
\$\endgroup\$
  • 1
    \$\begingroup\$ Thanks a bunch for this improvement! I actually ran into the bug in question and had no idea what was going on, thought it was just an imperfection with the Angular router but had not dug further. You're a savior. The bug I was running into was that I tried navigating to the same route and would get some could not subscribe to null error... \$\endgroup\$ – Lansana Feb 15 '18 at 18:35
2
\$\begingroup\$

A proposal with Subscription in returns to be able to unsubscribe all actions.

import { Injectable } from '@angular/core';
import {
    ActivatedRoute,
    Router,
    Event,
    NavigationStart,
    NavigationEnd,
    NavigationCancel,
    NavigationError,
    RoutesRecognized,
    RouteConfigLoadStart,
    RouteConfigLoadEnd
} from '@angular/router';

import { Observable, Subscription } from 'rxjs';
import 'rxjs/add/operator/map';
import 'rxjs/add/operator/filter';
import 'rxjs/add/operator/mergeMap';

/**
 * This service intercepts all routing requests going through Angular.
 * Source: https://codereview.stackexchange.com/questions/161783/angular2-route-interceptor
 */
@Injectable()
export class RouteInterceptor {
    private _events: Map<RouteInterceptorEvents, Observable<Event>> = new Map();

    constructor(
        private _activatedRoute: ActivatedRoute,
        private _router: Router
    ) {
        this._populateEventMap();
    }

    /**
     * Signs up a callback for when navigation starts.
     * @param callback The callback function to invoke.
     */
    onNavigationStart(callback: (router: ActivatedRoute) => void): Subscription {
        return this._getRouteOn(RouteInterceptorEvents.NavigationStart, callback);
    }

    /**
     * Signs up a callback for when navigation ends.
     * @param callback The callback function to invoke.
     */
    onNavigationEnd(callback: (router: ActivatedRoute) => void): Subscription {
        return this._getRouteOn(RouteInterceptorEvents.NavigationEnd, callback);
    }

    /**
     * Signs up a callback for when navigation is cancelled.
     * @param callback The callback function to invoke.
     */
    onNavigationCancel(callback: (router: ActivatedRoute) => void): Subscription {
        return this._getRouteOn(RouteInterceptorEvents.NavigationCancel, callback);
    }

    /**
     * Signs up a callback for when navigation errors.
     * @param callback The callback function to invoke.
     */
    onNavigationError(callback: (router: ActivatedRoute) => void): Subscription {
        return this._getRouteOn(RouteInterceptorEvents.NavigationError, callback);
    }

    /**
     * Signs up a callback for when configuration load starts.
     * @param callback The callback function to invoke.
     */
    onConfigLoadStart(callback: (router: ActivatedRoute) => void): Subscription {
        return this._getRouteOn(RouteInterceptorEvents.ConfigLoadStart, callback);
    }

    /**
     * Signs up a callback for when configuration load ends.
     * @param callback The callback function to invoke.
     */
    onConfigLoadEnd(callback: (router: ActivatedRoute) => void): Subscription {
        return this._getRouteOn(RouteInterceptorEvents.ConfigLoadEnd, callback);
    }

    /**
     * Signs up a callback for when the route is recongized.
     * @param callback The callback function to invoke.
     */
    onRouteRecognized(callback: (router: ActivatedRoute) => void): Subscription {
        return this._getRouteOn(RouteInterceptorEvents.RouteRecognized, callback);
    }

    private _getRouteOn(
        eventType: RouteInterceptorEvents,
        callback: (router: ActivatedRoute) => void
    ): Subscription {
        return this._events[eventType]
            // By returning a new Object (this.activatedRoute) into the stream, we
            // essentially swap what we're observing.
            // At this point we only run .map() if the filtered event (this.events[eventType])
            // successfully returns the event, meaning the event of 'eventType' has been triggered.
            .map(() => this._activatedRoute)
            // Traverse the state tree to find the last activated route, and then
            // return it to the stream. Doing this allows us to dive into the 'children'
            // property of the routes config to fetch the corresponding page title(s).
            .map(route => {
                while (route.firstChild) route = route.firstChild;
                return route;
            })
            // Filter un-named (primary) router outlets only.
            .filter(route => route.outlet === 'primary')
            .subscribe(route => callback(route));
    }

    private _populateEventMap() {
        const events = this._events;
        const routerEvents = this._router.events;

        events[RouteInterceptorEvents.NavigationStart] = routerEvents.filter(e => e instanceof NavigationStart);
        events[RouteInterceptorEvents.NavigationEnd] = routerEvents.filter(e => e instanceof NavigationEnd);
        events[RouteInterceptorEvents.NavigationCancel] = routerEvents.filter(e => e instanceof NavigationCancel);
        events[RouteInterceptorEvents.NavigationError] = routerEvents.filter(e => e instanceof NavigationError);
        events[RouteInterceptorEvents.ConfigLoadStart] = routerEvents.filter(e => e instanceof RouteConfigLoadStart);
        events[RouteInterceptorEvents.ConfigLoadEnd] = routerEvents.filter(e => e instanceof RouteConfigLoadEnd);
        events[RouteInterceptorEvents.RouteRecognized] = routerEvents.filter(e => e instanceof RoutesRecognized);
    }
}

enum RouteInterceptorEvents {
    NavigationStart,
    NavigationEnd,
    NavigationCancel,
    NavigationError,
    ConfigLoadStart,
    ConfigLoadEnd,
    RouteRecognized
};
\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.