Nested routers ergonomics

[owahltinez]

Related to #171 so not exactly an issue, since I think the documentation is pretty clear. But I really don't like having to declare the full path of nested routes. In my opinion, it greatly diminishes the usefulness of having nested routes at all.

Would you consider adding another method to Router, something like sub(router: Router), that walks through the child router's methods and attaches them to the parent router prepending the parent's base path?

If adding new methods to Router is a concern due to its minimal size, an alternative would be to create an auxiliary method like nest(parent: Router, child: Router) which does the same as described above — but that's a lot less elegant and is incompatible with the chaining API.

Another alternative is a new Router class that extends the minimal one (IttyRouter for the minimal one and BittyRouter for the extended one?), which adds the sub method described above. Since the original Router is untouched, the new class can be intended as the "batteries included" use case and other goodies like withParams can be included as well.

[kwhitley]

Yeah,

Related to #171 so not exactly an issue, since I think the documentation is pretty clear. But I really don't like having to declare the full path of nested routes. In my opinion, it greatly diminishes the usefulness of having nested routes at all.

Would you consider adding another method to Router, something like sub(router: Router), that walks through the child router's methods and attaches them to the parent router prepending the parent's base path?

The solution here isn't [likely] a new method... it's the fundamental path-matching itself, and how deep it's bakes into the router. One of the major issues here is that functions are passed around inside itty without awareness of which router they came from. For instance, when you do this:

router.all('/api/*', otherRouter.handle) 

You aren't passing the subrouter in, but rather the handle function itself. This handle function has no awareness of either its own router (upstream), or the outside router that called it (further upstream).

If adding new methods to Router is a concern due to its minimal size, an alternative would be to create an auxiliary method like nest(parent: Router, child: Router) which does the same as described above — but that's a lot less elegant and is incompatible with the chaining API.

The issue is not just the size from adding a method, it's breaking the very structure of the router proxy apart enough to add custom logic into it. Because this pathing logic is deep within the core of itty itself, it's also not a trivial thing to merely wrap as an extended router. Totally agree it would be nice (and the base path requirement is very non-standard from what folks expect coming from other routers). I've tried before on this challenge though, and will continue to see if I can solve it in an elegant way.

The next issue you would have though is all the existing routers out there - and how to handle their explicit base path vs the "new" style of auto-generating them (assuming you got past the size issue). I don't think you'd want to support both simultaneously in different routers either. So this would be arguably a "better" solution, but likely would be a drastically breaking change - which everyone hates for pretty obvious reasons.

Another alternative is a new Router class that extends the minimal one (IttyRouter for the minimal one and BittyRouter for the extended one?), which adds the sub method described above. Since the original Router is untouched, the new class can be intended as the "batteries included" use case and other goodies like withParams can be included as well.

Working on this one right now actually (it definitely includes withParams, auto formatting, error catching, etc)... hang tight!

[owahltinez]

What's wrong with something like:

/**
 * @param {RouterType} router
 * @param {RouterType} child
 */
function nest(router, child) {
  for (let [method, regex, handlers, path] of child.routes) {
    router[method](path, ...handlers);
  }
}

[owahltinez]

In line with the suggestion 3), this is what it would look like (and seems to work) but it would probably be nicer in typescript instead of using jsdoc annotations:

import { Router } from 'itty-router';

/**
 * @typedef {Object} BittyRouterOptions
 * @property {((...args: any[]) => Response)[]} middlewares default middlewares for all routes.
 */

/**
 * @typedef {Object} BittyRouterType
 * @property {import('itty-router').RouteEntry[]} routes
 * @property {(path: string, ...handlers: import('itty-router').RouteHandler[]) => BittyRouterType} all
 * @property {(path: string, ...handlers: import('itty-router').RouteHandler[]) => BittyRouterType} delete
 * @property {(path: string, ...handlers: import('itty-router').RouteHandler[]) => BittyRouterType} get
 * @property {(path: string, ...handlers: import('itty-router').RouteHandler[]) => BittyRouterType} head
 * @property {(path: string, ...handlers: import('itty-router').RouteHandler[]) => BittyRouterType} options
 * @property {(path: string, ...handlers: import('itty-router').RouteHandler[]) => BittyRouterType} patch
 * @property {(path: string, ...handlers: import('itty-router').RouteHandler[]) => BittyRouterType} post
 * @property {(path: string, ...handlers: import('itty-router').RouteHandler[]) => BittyRouterType} put
 * @property {(child: import('itty-router').RouterType | BittyRouterType, subpath: string?) => BittyRouterType} nest
 */

/**
 * Builds a Router with nested routing capabilities.
 * @param {BittyRouterOptions & import('itty-router').RouterOptions} options
 * @returns {BittyRouterType}
 */
export function BittyRouter(options = { middlewares: [] }) {
  const router = Router(options);

  for (let method of ['all', 'delete', 'get', 'head', 'options', 'patch', 'post', 'put']) {
    const fn = router[method];
    router[method] = (path, ...handlers) => fn(path, ...(options.middlewares || []), ...handlers);
  }

  /**
   * @param {import('itty-router').RouterType} child
   * @param {string?} subpath path relative to `base` for nested routes to be mounted.
   */
  router.nest = function (child, subpath) {
    for (let [method, , handlers, path] of child.routes) {
      router[method]((subpath || '') + path, ...handlers);
    }
    return router;
  };

  return router;
}

[kwhitley]

Not sure if you've seen the PR here (#208), but I'm working on a solution to this.

The end effect is code like this:

const childRouter = Router()
                       .get('/', () => 'child')
                       
const parentRouter = Router()
                       .get('/', () => 'parent')
                       .all('/child/*', childRouter) // passing the router, not the handle

The issue with the wrapper function mechanism you're proposing (and honestly a huge difficulty in general with implementing this, period), is that the regex to match is completely built at the time of route-creation. This becomes non-trivial to intercept and manipulate after-the-fact (like when being executed from within another route handler).

So far with that PR, for ~50 bytes, we have:

• the ability to pass routers as handlers, not just handles/handlers/middleware
• the ability to skip base paths when the nested route is simple (e.g. no route params in the path to the child router)
• for utmost speed, child base paths should still be used
• when you have route params or other advanced routes in the path to the child router, still have to use the base path

[owahltinez]

I'm not sure I understand what you mean by intercepting and manipulating the route. When a route is created, the original path is stored unmodified which lets you ignore the regex. Even better, by adding the child routes to the parent directly, you get even more speed (one less layer of indirection) than passing a child handle to the parent.

The sample code above is iterating over the child routes and adding them to the parent as the parent's routes.

[kwhitley]

Ahhh I see what you're doing - makes sense.

That said, by registering the child routes directly on the parent it may be faster, or may be slower. For very few routes (overall) it would likely be faster by unrolling the nesting - but when branching big routers, you can skip past entire (potentially huge) subtrees with a single regex check (where it doesn't go into the child router).

That said, I really do like your mechanism for iterating over the routes directly. It could potentially solve the issue where I can't handle advanced nesting routes, as the regex would be automatically recreated. I'm thinking by simply recreating the entire child router and attaching the handle, I cover all the cases, still keep the branching performance advantage, and may even save some bytes.

Back to experimenting! :)

[kwhitley]

After many passes, there's just not a byte-friendly and performance-friendly way to do this (that I've found).

Ultimately, in #208 I can get a relatively basic nesting working without base paths, but it comes with caveats (base paths are still more performant, base paths are required for fancier nesting routes, both options would need to be supported/documented instead of just one, etc).

My leaning is: shelve this for now... itty is one of the most performant edge serverless routers out there, period - largely due to its low byte size. This adds roughly 60 bytes for a slightly better ergo/DX in some cases (but still not recommended), yet adds no new functionality over what was available before. We def have to weigh the upside/downside on each of these choices, esp knowing once we pull the trigger, we're committed to supporting it going forward.

Benchmarks

https://github.com/TigersWay/cloudflare-playground

[kwhitley]

Until we find a more universal, low-byte, and performant means to do this, we're shelving this along with #208 for now. I'll love to revisit this if things change (because ofc it bugs all of us a little bit).