ukayani / restify-router

A router interface for restify that lets you aggregate route definitions and apply to a restify server
MIT License
52 stars 15 forks source link

Recursive routers #5

Closed Cyberuben closed 7 years ago

Cyberuben commented 7 years ago

I'm very used to the Express-way of handling routes. Is is possible to make these routers use another router?

The Express-way I'm talking about is as following:

// routes/routes.js
router.use("/v1", require("./v1/routes"));

// routes/v1/routes.js
router.use("/auth", require("./auth"));

// routes/v1/auth.js
router.post("/register", function (req, res, next) {});

The following code, in Express, would allow me to call POST /v1/auth/register and it'd simply work. Is the same possible using this module?

ukayani commented 7 years ago

Hi @Cyberuben,

That is definitely an interesting scenario. Currently there isn't any support for creating a tree like structure out of routers and applying that to the target restify server instance.

This is something that can be added in. I can try taking a stab at it when I get some time or if you can always submit a PR for this.

The way the .use method works for Restify is a little different than how it works for express. In express you can specify a path prefix for your common middleware however, in Restify you can only pass a middleware function. There is no .use(path, middleware) method in Restify.

Currently this router component does not create any middleware for you, it simply acts as a container for middleware/route definitions. To implement a recursive functionality, the router could be treated like a node in a tree, recursively applying routes for child routers when a call to applyRoutes is made.

Steinweber commented 7 years ago

+1

ukayani commented 7 years ago

@Cyberuben @Steinweber I've just added an implementation of this feature, please take a look at the PR i've tagged you both in. I'll cut a release after I updated the README.

Cheers

Steinweber commented 7 years ago
'use strict';

const router = require('restify-router').Router;
const routes = new router();

//returns restify-router instance
const service = require('./test');
const auth = require('./auth');

module.exports = (app) => {

    //Version
    const theApiV1 = new router();

    //api/v1
    theApiV1.add('/test',service);
    theApiV1.add('/auth',auth);
    theApiV1.applyRoutes(app, '/api/v1');

};

This works for me. I think its better to change the router.add params. Its a better usage to have the same order of params in all functions:

router.add(router,'prefix');
router.applyRoutes(instance,'optional prefix')

It´s also better for overview with (object, "on path")

Can u also add a merge function?

mainRouter.merge(subRouter);
mainRouter.merge(subrouter, subRouter2, ...);

example:

//Version
    const theApiV1 = new router();
    const theApiV2 = new router();

    const theApiBasic = new router();

    theApiBasic.add(auth,'/auth');
    theApiBasic.add(user,'/user');
    theApiBasic.add(foo,'/foo');

    theApiV1.merge(theApiBasic);
    theApiV1.add(service, '/service');
    theApiV1.applyRoutes(app, '/api/v1');

    theApiV2.merge(theApiBasic);
    theApiV2.add(bar, '/bar')
    theApiV2.applyRoutes(app, '/api/v2');

Big thanks for the realy fast update :)

ukayani commented 7 years ago

@Steinweber Thanks for testing it out.

Regarding the ordering of the parameters the reason I kept it as (path, router) is because I'm treating path as a required parameter. It also follows a similar pattern as the other calls such as

router.get(path, handler)
router.post(path, handler)

In the case of add, instead of adding a handler you are adding another router ( a grouping of handlers)

For applyRoutes, prefix comes after the server instance because it is optional.

Could you please create a separate issue for the merge feature?

It seems fairly straightforward to add in. I had a some questions about the exact behaviour you are looking for with merge.

const apiV1 = new Router();
const apiV2 = new Router();

const shared = new Router();

shared.get('/member', (req, res, next) => {});
shared.get('/posts', (req, res, next) => {});

const foo = new Router();
foo.get('/foo', (req, res, next) => {});

const bar = new Router();
bar.get('/bar', (req, res, next) => {});

apiV1.merge(shared);
apiV1.add('/misc', foo);

apiV2.merge(shared);
apiV2.add('/misc', bar);

apiV1.applyRoutes(server, '/api/v1');
apiV2.applyRoutes(server, '/api/v2');

with the above v1 and v2 definitions, if i'm understanding correctly, you want the following routes registered:

GET /api/v1/member
GET /api/v1/posts
GET /api/v1/misc/foo

GET /api/v2/member
GET /api/v2/posts
GET /api/v2/misc/bar

Is that the behaviour you'd expect?

If that is indeed the behaviour, it seems like router.merge(r2) <-> router.add('', r2). In other words, a merge is simply an add with an empty path.

ukayani commented 7 years ago

Implemented in v0.4.0