feat: add nested router support with mount() and Router class#396
feat: add nested router support with mount() and Router class#396avoidwork wants to merge 1 commit into
Conversation
- Add Router class with chainable route methods (get, post, put, delete, patch, always, etc.) - Add app.mount(path) to create a composable router under a shared prefix - Add app.use(path, router) to mount a Router instance with prefix flattening - Support recursive nested routers (Router within Router within Router...) - Support parameterized routes (/:id) working through mount chains - Keep error handlers isolated per Router scope - Add TypeScript definitions for Router class and new method signatures - Add 41 unit tests across 7 test files for router functionality
🤖 Augment PR SummarySummary: This PR introduces nested routing support by adding a composable Changes:
Technical Notes: Nested routers are flattened at mount time by prefixing paths and registering them into the parent middleware map; this preserves the existing regex-based matcher while enabling hierarchical route organization. 🤖 Was this summary useful? React with 👍 or 👎 |
![augmentcode[bot]](https://avatars.githubusercontent.com/in/1027498?s=60&v=4){kind=small}
| fullPath = path.startsWith(SLASH) ? path : `${SLASH}${path}`; | ||
| } else { | ||
| const p = path.startsWith(SLASH) || path === "" ? path.substring(INT_1) : path; | ||
| fullPath = `${prefix}${SLASH}${p}`; |
There was a problem hiding this comment.
src/middleware.js:L266: fullPath = ${prefix}/${p}`` will produce double slashes when prefix is `"/"` (e.g. mounting at root yields `//users`), and it also doesn't enforce that `prefix` begins with a leading slash, which can register routes that will never match.
Severity: high
🤖 Was this useful? React with 👍 or 👎, or 🚀 if it prevented an incident/outage.
| this.#logger.logMiddleware(rpath, fn[fn.length - INT_1]); | ||
| if (fn.length === INT_1 && isRouter(fn[INT_0])) { | ||
| const router = fn[INT_0]; | ||
| const prefix = typeof rpath === STRING ? rpath : SLASH; |
There was a problem hiding this comment.
src/woodland.js:L768: When mounting a Router, prefix falls back to SLASH if rpath isn't a string, which can silently mount at / and ignore the provided first argument if use() is called with an unexpected signature.
Severity: medium
🤖 Was this useful? React with 👍 or 👎, or 🚀 if it prevented an incident/outage.
| * @param {string} path - Mount prefix path | ||
| * @returns {Router} Chainable router instance | ||
| */ | ||
| mount(path) { |
There was a problem hiding this comment.
src/woodland.js:L803: mount(path) stores path on Router.prefix, but that prefix is never used during route registration/flattening (the effective prefix comes from the app.use(rpath, router) call), so mount('/api') doesn't actually compose paths unless the caller repeats the prefix.
Severity: medium
🤖 Was this useful? React with 👍 or 👎, or 🚀 if it prevented an incident/outage.
| * @returns {Router} Returns self for chaining | ||
| */ | ||
| ignore(fn) { | ||
| this.parent.ignore(fn); |
There was a problem hiding this comment.
src/woodland.js:L833: Router.ignore() assumes this.parent is a Woodland instance with ignore(), but the PR's own tests construct nested routers with new Router(apiRouter, ...), where parent is another Router and this method would throw if used.
Severity: medium
🤖 Was this useful? React with 👍 or 👎, or 🚀 if it prevented an incident/outage.
| } | ||
|
|
||
| const pathMap = this.routeMap.get(method); | ||
| const last = fn[fn.length - INT_1]; |
There was a problem hiding this comment.
src/woodland.js:L945: #registerPath() allows registering a route with no handlers (empty fn), which later mounts into the middleware registry as a route with zero handlers and can lead to confusing "registered but does nothing" behavior.
Severity: medium
🤖 Was this useful? React with 👍 or 👎, or 🚀 if it prevented an incident/outage.
| mount(path: string): Router; | ||
| use(rpath: string | Function, ...fn: Function[]): Woodland; | ||
| use(rpath: string, ...fn: (Function | string)[]): Woodland; | ||
| use(rpath: string, subApp: Woodland): Woodland; |
There was a problem hiding this comment.
types/woodland.d.ts:L85: The overload use(rpath: string, subApp: Woodland) doesn't match the runtime implementation, which only detects/handles Router instances in Woodland.use() (passing a Woodland instance here won't be treated as a nested router).
Severity: high
🤖 Was this useful? React with 👍 or 👎, or 🚀 if it prevented an incident/outage.
| export default woodland; | ||
|
|
||
| export class Router { | ||
| parent: Woodland; |
There was a problem hiding this comment.
types/woodland.d.ts:L102: Router.parent is typed as Woodland, but the PR's tests create nested routers with a Router parent; this type mismatch will make that supported pattern hard/impossible to type-check.
Severity: medium
🤖 Was this useful? React with 👍 or 👎, or 🚀 if it prevented an incident/outage.
| The system SHALL allow mounting a Woodland instance at a given path prefix so that all of its routes are resolved relative to that prefix. | ||
|
|
||
| #### Scenario: Mount sub-app at prefix | ||
| - **WHEN** a Woodland instance is passed as the second argument to `app.use('/api', subApp)` |
There was a problem hiding this comment.
openspec/changes/nested-routers/specs/nested-routers/spec.md:L7: This spec states app.use('/api', subApp) mounts a Woodland instance, but the implementation added in this PR mounts only Router instances; the spec and code should agree on what the supported nested unit is.
Severity: medium
🤖 Was this useful? React with 👍 or 👎, or 🚀 if it prevented an incident/outage.
| export const ROUTE_PATTERN = "(/.*)?"; | ||
| export const MSG_USE_MIDDLEWARE_REQUIRED = | ||
| "useMiddleware is required or config.use must be a function"; | ||
| export const MSG_SUB_APP_REQUIRED = "Expected a Woodland instance for nested routing"; |
There was a problem hiding this comment.
src/constants.js:L156: MSG_SUB_APP_REQUIRED is introduced but not used anywhere in the runtime checks, which makes it easy for invalid nested-routing inputs to fail with less-specific errors (or silently mount at /).
Severity: low
🤖 Was this useful? React with 👍 or 👎, or 🚀 if it prevented an incident/outage.
| ℹ fileserver.js | 100.00 | 89.36 | 100.00 | | ||
| ℹ logger.js | 100.00 | 94.23 | 95.45 | | ||
| ℹ middleware.js | 100.00 | 100.00 | 100.00 | | ||
| ℹ middleware.js | 99.72 | 96.59 | 100.00 | 263 |
There was a problem hiding this comment.
coverage.txt:L11: The committed coverage report shows new uncovered lines (e.g. src/middleware.js at 99.72%), which appears to conflict with the repo's stated requirement of 100% line coverage (Rule: AGENTS.md).
Severity: medium
🤖 Was this useful? React with 👍 or 👎, or 🚀 if it prevented an incident/outage.
1 participant
Description
Adds nested router capability to Woodland, enabling developers to break up app concerns into modular, hierarchical route trees through `app.mount(path)` and `app.use(path, router)` APIs.
Type of Change
Testing
Ran the full test suite before committing:
New test files:
Coverage
Checklist
Affected Files