Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

addRoutes() adds unusable router and extend() will loose all prefixes #23

Open
BananaAcid opened this issue Aug 16, 2017 · 15 comments
Open

Comments

@BananaAcid
Copy link

Adding a let r2 = new Router({prefix: 'a'}).loadMethods().get('/', (ctx, next) => {ctx.body='supposed to be at /api/a/'; return next();}) with let r1 = new Router({prefix: 'api'}).addRoutes(r2); does not result in a callable route. Actually koa-better-router crashes with not having any routes.

And using extend -> let r1 = new Router({prefix: 'api'}).extend(r2); will loose all prefix info from r2. Putting all '/' routes at the same base level.

Any known workaround?

@BananaAcid BananaAcid changed the title addRoutes adds unusable router addRoutes() adds unusable router and extend() will loose all prefixes Aug 16, 2017
@tunnckoCore
Copy link
Owner

Hi! Thanks for opening that.

What's your specific case and real world example? I'm a bit tired of this composition problems that are always reported. And i feel that in most cases they are just confusion with the docs and the understandings how they work in reality.

Using .extend is expected to save only one prefix. Probably if you share what's your case and what routes you should have, i can help with something. Try .groupRoutes?

@tunnckoCore
Copy link
Owner

tunnckoCore commented Aug 16, 2017

Oh, and one more thing: .addRoutes accept Route Objects as arguments, not routers - see the docs. This means that you can only pass an object that is created from .createRoute method.

@BananaAcid
Copy link
Author

My basic question is basically: how do i load Routes from files with prefixes to be sorted into other routes with prefixes. Like a basic router(prefix:'/') that gets added ones: [router(prefix:'/webapp')+routes and router(prefix:'/api')]. where api gets router(prefix:'/v1')+routes and router(prefix:'/latest') the same as v1.
all within their respective folders to group them.

how would i do that to end up with:
/ webapp/...
/ api/ v1/...
/ api/ latest/...

@tunnckoCore
Copy link
Owner

Hm, that clarify the things a bit. One more note that may worth: you don't need to merge all routers, but to call .middleware() multiple times, like

const router1 = new Router()
const router2 = new Router()
const router3 = new Router()

app.use(router1.middleware())
app.use(router2.middleware())
app.use(router3.middleware())

Try that, otherwise we'll try something other, i have some ideas, but probably this is the easiest.

@BananaAcid
Copy link
Author

BananaAcid commented Aug 17, 2017

thought about this, but it would require either the each router to have a full prefix path or have each route to include the full path.

what i am currently trying to do is, overwrite your router object and modify your addRoutes to resolve each router.routes[] with your utils.updatePrefix() to import/append them as routes to each "parent" / addRoutes caller. (which kills all possibilities to go with managing routes of the originating router)
The other thing I thought about is, how to handle an added router object added with addRoutes to resolve when used. but that's just an idea. And lastly if extend could append the child prefix.

@tunnckoCore
Copy link
Owner

Can you share some examples/snippets? And, would you have some routes on the / prefix (except /webapp and /api)?

@tunnckoCore
Copy link
Owner

tunnckoCore commented Aug 17, 2017

See that

'use strict'

const Router = require('./index')
const Koa = require('koa')

const R = new Router({ prefix: '/' }).loadMethods()
const routerApp = new Router({ prefix: '/webapp' }).loadMethods()
const routerApiV1 = new Router({ prefix: '/api/v1' }).loadMethods()
const routerApiLatest = new Router({ prefix: '/api/latest' }).loadMethods()

const app = new Koa()

R.get('/', (ctx, next) => {
  ctx.body = `Hello world! Web`
  return next()
})

R.get('/about', (ctx, next) => {
  this.body = `About company`
  return next()
})

console.log('root', R.getRoutes())
console.log('======')

routerApp.get('/', (ctx, next) => {
  ctx.body = 'Home page of WebApp'
  return next()
})

routerApp.get('/users', (ctx, next) => {
  ctx.body = 'Users of WebApp'
  return next()
})

console.log('webapp', routerApp.getRoutes())
console.log('======')

routerApiV1.get('/', (ctx, next) => {
  ctx.body = 'API v1 home'
  return next()
})

routerApiV1.get('/repos', (ctx, next) => {
  ctx.body = 'User repos'
  return next()
})

routerApiV1.get('/gists', (ctx, next) => {
  ctx.body = 'User gists'
  return next()
})

console.log('API Version 1', routerApiV1.getRoutes())
console.log('======')

routerApiLatest.get('/', (ctx, next) => {
  ctx.body = 'Latest API endpoint, dont use in production'
  return next()
})

routerApiLatest.get('/repositories', (ctx, next) => {
  ctx.body = 'New version of old v1 /repos endpoint'
  return next()
})

routerApiLatest.get('/issues', (ctx, next) => {
  ctx.body = 'Repository issues'
  return next()
})

console.log('API Latest', routerApiLatest.getRoutes())
console.log('======')

app.use(R.middleware())
app.use(routerApp.middleware())
app.use(routerApiV1.middleware())
app.use(routerApiLatest.middleware())

@tunnckoCore
Copy link
Owner

tunnckoCore commented Aug 17, 2017

You probably didn't tried to set /api/v1 as prefix and the other as /api/latest. It's not a problem to have more than one slash. And maybe that's the problem.

@BananaAcid
Copy link
Author

BananaAcid commented Aug 17, 2017

you might want to check this out:
https://gist.github.com/BananaAcid/c6e1939ceef9ec48cbc4a7b3117c28d1
-> ˫ routes ˫ api1 ˫ index.js

@tunnckoCore
Copy link
Owner

tunnckoCore commented Aug 17, 2017

Don't use .extend, it's probably not your case. I'll said it again, try .groupRoutes and instead of VERB methods or .addRoute, use .createRoute - it just creates objects. And one more time, don't use one single router and everything (other routers) merged in it.

Is result file there, the expected result or is current result? Sorry, i just still don't get the endpoints architecture.

btw,

// add routes - hack (keep prefixes, but does not merge them with local one, but local one would be '/' anyways)  !!
app.routes.forEach( (e) =>
	router.routes.push(e)
);

that is exactly what .addRoutes (plural) does, so

app.addRoutes(router.routes)
api1.addRoutes(router.routes)

It accepts any number of arguments and array of route objects too. (i believe)

@tunnckoCore
Copy link
Owner

tunnckoCore commented Aug 17, 2017

Hmmmm. Okey, start to getting the situation. Probably new preservePrefix option to .extend method would be good add.

@tunnckoCore
Copy link
Owner

Yep, easy. I just replcated your gist structure and everything, and implemented preservePrefix option for the .extend method. Given that api1/index.js which includes routes from api1/a.js router and routes from api1/b.js router.

'use strict'

const Router = require('koa-better-router')

const AAA = require('./a')
const BBB = require('./b')

const APIv1 = new Router({
  prefix: '/api/v1',
}).loadMethods()

APIv1.get('/', (ctx, next) => {
  ctx.body = router.routes.map(e => e.path)
  return next()
})

APIv1.get('/x', (ctx, next) => {
  ctx.body = 'list ...'
  return next()
})

APIv1.extend(AAA, { preservePrefix: true })
APIv1.extend(BBB, { preservePrefix: true })

APIv1.getRoutes().forEach(route => {
  console.log(route)
  console.log('========')
})

module.exports = APIv1

the result is the following

{ prefix: '/api/v1',
  path: '/api/v1/',
  route: '/',
  match: [Function],
  method: 'GET',
  middlewares: [ [Function] ] }
========
{ prefix: '/api/v1',
  path: '/api/v1/x',
  route: '/x',
  match: [Function],
  method: 'GET',
  middlewares: [ [Function] ] }
========
{ prefix: '/api/v1',
  path: '/api/v1/a/obj/:one/:two',
  route: '/a/obj/:one/:two',
  match: [Function],
  method: 'PUT',
  middlewares: [ [Function] ] }
========
{ prefix: '/api/v1',
  path: '/api/v1/a/obj/:one',
  route: '/a/obj/:one',
  match: [Function],
  method: 'GET',
  middlewares: [ [Function] ] }
========
{ prefix: '/api/v1',
  path: '/api/v1/b/obj/:one/:two',
  route: '/b/obj/:one/:two',
  match: [Function],
  method: 'PUT',
  middlewares: [ [Function] ] }
========
{ prefix: '/api/v1',
  path: '/api/v1/b/obj/:one',
  route: '/b/obj/:one',
  match: [Function],
  method: 'GET',
  middlewares: [ [Function] ] }
========

is that looks okey?

@tunnckoCore
Copy link
Owner

tunnckoCore commented Aug 18, 2017

Basically, that's the updated code of .extend

KoaBetterRouter.prototype.extend = function extend(router, options) {
  if (!(router instanceof KoaBetterRouter)) {
    throw new TypeError(
      '.extend: expect `router` to be instance of KoaBetterRouter'
    )
  }
  const opts = Object.assign({ preservePrefix: false }, options)

  const updateOnPreserve = (route, target) => {
    route.prefix = target.prefix
    route.route = route.path
    route.path = route.prefix + route.path

    // rebuild the matcher function
    // to be against the new route path (which includes the target prefix)
    route.match = router.route(route.path)
    return route
  }

  router.routes.forEach(route => {
    if (route.prefix !== this.options.prefix) {
      route = opts.preservePrefix
        ? updateOnPreserve(route, this.options)
        : utils.updatePrefix(this, this.options, route)
    }

    this.routes.push(route)
  })
  return this
}

you can replace it and see if everything works as expected.

There are more breaking changes that should be done, so i will update that too.

@BananaAcid
Copy link
Author

Thanks for sorting this out 👍
I had no chance yet, to get back at my project. But will try that monkey-patch at the weekend. When will it go into the repo/NPM?

great stuff btw!

@tunnckoCore
Copy link
Owner

Thanks to you too! Open source is great thing and lovely place 🎉

When will it go into the repo/NPM?

I'll try as soon as possible. Probably on the weekend, at least.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants