baseprime / grapnel

The smallest JavaScript router with named parameters, HTML5 pushState, and middleware support
http://grapnel.js.org
467 stars 40 forks source link

Implement `router.use()` method #37

Closed kenvunz closed 8 years ago

kenvunz commented 9 years ago

Issue #31

baseprime commented 9 years ago

Thanks for this! Any reason why you decided on a global stack instead of invoking router.add?

kenvunz commented 9 years ago

is this what you mean by invoking router.add?

Grapnel.prototype.use = function() {
    var route, fn;
    if(arguments[0] && arguments[1]) {
        route = arguments[0];
        fn = arguments[1];
    } else {
        route = '/*';
        fn = arguments[0];
    }

    //fn.route = Grapnel.regexRoute(route);

    //this.stack.push(fn);

    this.add(route, fn);
};
baseprime commented 9 years ago

The issue is that middleware needs to be a stack. The component that is missing in your code is the ability to call to the next middleware in the stack, or to end the cycle. Take the following example of the ideal usage of router.use:

router.use(function(req, event, next){
    user.auth(function(err){
        if(err){
            throw new Error(err);
            return;  // End cycle
        }

        req.user = this;
        next(); // Call to next in stack
    });
});
kenvunz commented 9 years ago

ah sure, i know what you mean now. Have you had a look at the my added tests? Doesn't it satisfy what you described above?

I can add more tests in too if required

test('Middleware defined in use() works correctly', function(assert) {
    var done = assert.async(),
        r = new Grapnel({pushState: true}),
        testObj = {},
        lastInStackCalled = false;

    // wildcard middleware, implying `/*`
    r.use(function(req, event, next) {
        testObj.fn1 = true;
        next();
    });

    // specific route middleware
    r.use('/a', function(req, event, next) {
        testObj.fn2 = true;
        next();
    });

    // this should not be triggered
    r.use('/b', function(req, event, next) {
        testObj.fn3 = true;
        next();
    });

    r.get('/a', function(req, event) {
        lastInStackCalled = true;
    }).navigate('/a');

    setTimeout(function() {
        equal(testObj.fn1, true);
        equal(testObj.fn2, true);
        equal(testObj.fn3, undefined);
        equal(lastInStackCalled, true);
        done();
    }, 500);
});
kenvunz commented 9 years ago

In saying that, i've just found a bug in my approach is that it's not possible to execute the middleware stack without defining a route, but let make sure you are happy with my approach first then i'll work on the fix :)