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

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

Closed
noinkling opened this issue Feb 8, 2022 · 2 comments

Comments

@noinkling
Copy link
Contributor

noinkling commented Feb 8, 2022

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:
    return new Error(`No route found for name: ${name}`);

    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
Copy link
Contributor

titanism commented Jul 4, 2022

Closed via #143

@titanism titanism closed this as completed Jul 4, 2022
@titanism
Copy link
Contributor

titanism commented Jul 4, 2022

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.

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

No branches or pull requests

2 participants