koajs / router

Router middleware for Koa. Maintained by @forwardemail and @ladjs.
MIT License
849 stars 174 forks source link

.url() and other methods that take route names are awkward to use with nested routers because of name collisions #142

Closed noinkling closed 2 years ago

noinkling commented 2 years ago

node.js version: 16.13.2

@koa/router version: 10.1.1

koa version: 2.13.4

Explanation:

In order to get the full path of a named route in a nested router using the .url() method, we have to call it on the base router (same principle applies for .redirect() and .route()):

// index.js

const Koa = require('koa');
const Router = require('@koa/router');

const fooRouter = require('./fooRouter');

const app = new Koa();

const apiRouter = new Router({ prefix: '/api' });

apiRouter.use('/v1', fooRouter.routes());
app.use(apiRouter.routes());

app.listen(3000, () => console.log('Listening...'));
// fooRouter.js

const Router = require('@koa/router');

const foos = {};
let nextFooId = 1;

const fooRouter = new Router();

fooRouter.post('create', '/foos', (ctx, next) => {
  foos[nextFooId] = { id: nextFooId, foo: 'foo' };
  ctx.status = 201;

  // Here I want to get the 'read' route for this resource so I can
  // send it back to the client, e.g. in the `Location` header or body

  console.log(fooRouter.url('read', { id: nextFooId }));  // '/foos/1' - incomplete path

  // Need to use the base router in order to get the full path:
  console.log(ctx.router.url('read', { id: nextFooId }));  // '/api/v1/foos/1'

  console.log(fooRouter.route('read') === ctx.router.route('read'))  // false

  nextFooId++;
});

fooRouter.get('read', '/foos/:id', (ctx, next) => {
  ctx.body = foos[id];
});

module.exports = fooRouter;

This is a little bit of a pain to remember, but makes sense when you consider that a nested router can't know where it's mounted (at least, not until request time), especially since it could potentially be mounted multiple times in different places. I think a note about this should probably be made in the docs, especially considering that in some previous versions (I know in 7.4.0 at least), calling .url() did actually give the full path... as long as you only mounted each router only once, otherwise it was very broken. It's easy to not realise that the behaviour changed.

That's only a minor issue, the bigger problem is that since we have to use the base router, the names we give to any routes have to be unique across the whole routing tree, or else there's no guarantee we'll get the one we expect. For example if we re-use fooRouter for v2 of the API like so:

// index.js

const Koa = require('koa');
const Router = require('@koa/router');

const fooRouter = require('./fooRouter')

const app = new Koa();

const apiRouter = new Router({ prefix: '/api' });

apiRouter.use('/v1', fooRouter.routes())
apiRouter.use('/v2', fooRouter.routes())
app.use(apiRouter.routes())

app.listen(3000, () => console.log('Listening...'))

a POST request to http://localhost:3000/api/v2/foos will log the v1 path instead.

Additionally, if we created a barRouter with the same structure as fooRouter (maybe we're using an abstraction like a factory to create routers for resources), we could run into a similar collision where the path for a bar resource is returned instead of a foo, or vice versa.

Workaround:

Since the route identifiers need to be globally unique, on a whim I tried using symbols in place of strings, which turns out to already work as-is! All you have to do is make sure you're not sharing references to the same symbol. The only issues I found with this approach are:

  1. If the route attempting to be referenced doesn't exist, an error will be thrown because of this line: https://github.com/koajs/router/blob/90dd73c44d0e76db0890b552023711de896c12d3/lib/router.js#L656 Symbols can't be implicitly converted to strings, so name would just need to be wrapped in String() or similar.
  2. It's slightly more verbose
  3. It feels weird that it's necessary

Do you think this approach should be supported/documented (since there's almost no work needed), or should we look for a way to solve the issue that's a little more ergonomic?

titanism commented 2 years ago

Closed via #143

titanism commented 2 years ago

We have just published @koajs/router v11.0.0 which resolves this issue. This is mirrored to koa-router as well.

https://github.com/koajs/router/releases/tag/v11.0.0

This project is maintained by Forward Email and Lad.