-
-
Notifications
You must be signed in to change notification settings - Fork 117
feat: add getRoutes method to list registered routes #174
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
base: master
Are you sure you want to change the base?
Conversation
Signed-off-by: Sebastian Beltran <bjohansebas@gmail.com>
|
We need clarification on the exact requirements. Can you have them open a feature request? There are different things that could be exposed, which of these is the request? 1. what routes did a user register?
2. what will match what the user registered?
If the ask is number 2, they would need to decide if they apply the routing rules themselves, tell users which options are supported, or we'd need to enhance the ouput to include either router options or otherwise take options into account. (v good PR description ❤️) Edit: More importantly, we'd want to answer this for ourselves if we add this feature. If folks depend on this they'll need to know what information is actually conveyed by the returned mapping. Strings they used, or strings router will recognize.Current approach flattens the whole stack, so we are losing the nuance that different routers can be configured differently. Sweet spot between the two options could be expressed in output as: [
{
path: '/api',
methods: ['GET'],
options: { strict: false, caseSensitive: false }
},
{
path: '/v1/users',
methods: ['GET'],
options: { strict: true, caseSensitive: true }
}
] |
jonchurch
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
|
@jonchurch thanks for the quick review! Yes, what they shared with me (and I also asked them to comment here about their needs) is that they want to be able to see the routing structure and then map it to their Build Output API (https://vercel.com/docs/build-output-api). In my opinion, the solution you're proposing (combining both outputs) is also the right one, as it covers those needs as well as other use cases or certain hosting providers. Plus, it's flexible enough for developers to adapt it to their specific requirements. [
{
path: '/api',
methods: ['GET'],
options: { strict: false, caseSensitive: false }
},
{
path: '/v1/users',
methods: ['GET'],
options: { strict: true, caseSensitive: true }
}
]
``` ref https://github.com/pillarjs/router/pull/174#issuecomment-3129927730 |
Signed-off-by: Sebastian Beltran <bjohansebas@gmail.com>
Signed-off-by: Sebastian Beltran <bjohansebas@gmail.com>
Signed-off-by: Sebastian Beltran <bjohansebas@gmail.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I left a few comments, but I think we should workshop this just a bit. I think ideally we don't need to do this work when we call routes() but instead we can keep internal track of these in such a way that they are much more easily operated on. That would likely only be possible in the next major, but if we can design this api such that it is our "ideal internal book keeping structure" then we can ease that transition in the next breaking change.
So I think my ask is that we add some usage examples in the main text of the PR (or maybe in the readme as docs in this pr) and then we can propose some small changes to the exposed api that way before we invest too much more work in the internals and tests?
EDIT: also, I was thinking that maybe one good pressure test for the api would be if I tried to integrate it into my openapi generator package. If it works well for that, I think it would work well for all the similar use cases. If we can organize this work I am happy to make it a priority.
|
I have thoughts on this PR, but won't have time to review for a bit. If it's urgently needed, I don't want to be the blocker, but otherwise would like time to review. 🙏 |
It's not entirely urgent, so feel free to do the review whenever you can. |
blakeembrey
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks like a great starting point, love it! @jonchurch has some really good feedback to think about here too, re what Vercel may need.
Can Vercel handle the regex support?
| } | ||
|
|
||
| // for layers with a .use (mounted routers) | ||
| if (layer.pathPatterns && layer.handle && layer.handle.stack && !layer.route) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For .use, could it make sense to allow consumers to iterate recursively themselves? It's something that could be added in a follow up, but the MVP could be simply { path, method, router }. Every field could also be optional, I guess, since path: undefined with .use(fn), method: undefined when all is used, and router: undefined when no nested router.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The path will never be undefined .use always sets the path to '/' when no path is explicitly provided in the arguments.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's the kind of thing that should be nailed down for the API, but understood. It probably is reasonable to keep it as / and the object was intended to be hypothetical.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
E.g. why / vs ""? Does one make it harder for consumers than the other? How do these interact with internal routing behaviors that aren't being exposed in this API? How much can move these expectations/behaviors to be static instead of magic (e.g. removing the trailing / is the one that comes to mind, people need to know how the package works internally).
Signed-off-by: Sebastian Beltran <bjohansebas@gmail.com>
…nstead of having opinions about it Signed-off-by: Sebastian Beltran <bjohansebas@gmail.com>
Signed-off-by: Sebastian Beltran <bjohansebas@gmail.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Mostly looks good to me now. However, I wonder if there's possibility of infinite recurrsion at collectRoutes.
Imagine a user acceddientally creates circular route references and then calls to getRoutes would go on forever.
Perhaps there should a protection, maybe check if the current layer is visited in the current branch. Here's a possible sample code,
function collectRoutes (stack, prefix, routeMap, options, visited = new WeakSet()) {
for (const layer of stack) {
if (layer.pathPatterns && layer.handle && layer.handle.stack && !layer.route) {
// Check for circular reference
if (visited.has(layer.handle)) {
continue // Skip circular reference
}
visited.add(layer.handle)
collectRoutes(layer.handle.stack, pathPrefix, routeMap, options, visited)
visited.delete(layer.handle) // Allow re-visiting in different branches
}
}
}Other than this, I think getRoutes is awesome feature to the router.
Good work @bjohansebas 👏
cc: @blakeembrey @wesleytodd @jonchurch
Update
Just to be safe, @bjohansebas can we add more complex and nested routes in the test often found in real world scenarios to be safe?
|
Note: options will need to contain |
|
@bjohansebas you're my hero; eagerly awaiting this change. |
|
Thank you for your work on this pull request. While I was working on: wesleytodd/express-openapi#77, I had to rely on the changes in this PR to get the full path of the routes. Would it be possible to expose the middleware stack of each route within |
|
Yep, that could be added in my opinion. I need to partially modify this PR to adapt it to the idea in #174 (comment), I have it in my priorities, so you’ll need to adapt your PR to that format once I make the change. |
This adds support for an experimental builder that does app introspection on the Express instance. It can used with an env var `VERCEL_EXPERIMENTAL_EXPRESS_BUILD=1`. It provides route-level o11y so when you're looking at your logs you can see the routes served <img width="261" height="306" alt="Screenshot 2025-09-19 at 3 32 30 PM" src="https://github.com/user-attachments/assets/47d01c66-6bf4-4229-8339-fd75625c8a02" /> Key differences: - No longer calls into the `@vercel/node` builder. Uses `rolldown` instead. - It does NOT check types at this time. - It still doesn't bundle, it just transforms the files in-place. And doesn't process `node_modules` aside from using NFT for them, as we do in the node builder. - Using rolldown mostly because what we need to do here has diverged from the `@vercel/node` logic sufficiently and starting from scratch is a bit easier for now. And it had some wins over `esbuild` for being able to avoid bundling. - It has the added benefit of working with tsconfig paths, missing extensions on relative imports, and code that mixes esm/cjs syntax. - NFT is run on the compiled result instead of source, it wasn't resolving tsconfig path imports like `@/my-lib` or imports with missing file extensions, but running it against the compiled code seems to work nicely. - Writes result to the filesystem, whereas with the node builder everything is in memory until `writeBuildResult`. - Invokes the compiled code with a monkey-patched version of express that emits app metadata like routes, static folders and view folders. - Uses the build output version 2, there's some messy code in the build command to handle this because the `@vercel/express` package currently uses version 3 - Does not support `vc dev` at the moment. Use `srvx`! ### Explanation The gist of the POC is that we need to extract info from the express instance, so we compile the source code, monkey-patch express with a wrapper function that exports the app's metadata, invoke the code with a child process and then use the metadata add routes to the build output and then clean up the monkey-patch code. NOTE: the introspection logic is basic, it doesn't process sub-routes. There's some work to the [Express router](pillarjs/router#174) that will make this easier for v5, and for v4 the logic will look different but AFAIK it's easier to do there. ### Before we can really use this as the main path forward We need to validate this approach, it's kind of a hack but does give us some nice improvements. The change to using rolldown is arguably not necessary, we might want to continue to use the node builder logic to reduce friction. --------- Co-authored-by: vercel[bot] <35613825+vercel[bot]@users.noreply.github.com>
|
👋 we (Vercel) have just added an experimental feature in our Express builder that allows us to extract metadata from the Express app like routes, static assets folders, and view rendering engines so we can build Express apps with first-class features like o11y out of the box. Here's the initial PR with more details, but at the moment the route metadata is very basic (doesn't capture sub-routers) so I wanted to check in on this PR and ask if there's anything we can do to make route introspection better. You can try it out by using this repo, there's a Vercel deploy button in the README. |
|
@bjohansebas - Anything we can help with to get this shipped? We're excited to leverage this on Vercel's side. |
Signed-off-by: Sebastian Beltran <bjohansebas@gmail.com>
Signed-off-by: Sebastian Beltran <bjohansebas@gmail.com>
Signed-off-by: Sebastian Beltran <bjohansebas@gmail.com>
Signed-off-by: Sebastian Beltran <bjohansebas@gmail.com>
Signed-off-by: Sebastian Beltran <bjohansebas@gmail.com>
Signed-off-by: Sebastian Beltran <bjohansebas@gmail.com>
Signed-off-by: Sebastian Beltran <bjohansebas@gmail.com>
|
Hey, sorry for the delay, i’ve been away from open source for a while, and I’m slowly getting back to catching up on all the pending work. @tknickman and express community, it would be great to know if you have any thoughts on how this API should be presented. We can’t cover every possible route style that this library allows, but we can try to handle the majority of them. What I still have pending to do is:
|
Signed-off-by: Sebastian Beltran <bjohansebas@gmail.com>
c130861 to
306eefd
Compare
Signed-off-by: Sebastian Beltran <bjohansebas@gmail.com>
Signed-off-by: Sebastian Beltran <bjohansebas@gmail.com>
IamLizu
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey @bjohansebas!
I think there's infinite recursion on cyclic routers (index.js lines ~479-548). collectRoutes walks mounted routers without a visited-set, so any accidental cycle (reusing routers across mounts) makes getRoutes() blow the stack. For example,
const Router = require('./')
const app = new Router()
const admin = new Router()
const account = new Router()
admin.get('/dashboard', () => {})
account.get('/:accountId', () => {})
app.use('/admin', admin)
app.use('/accounts', account)
// Later refactor reuses routers and creates a cycle:
admin.use('/accounts', account)
account.use('/admin', admin)
app.getRoutes() // RangeError: Maximum call stack size exceededMaybe add a WeakSet (or similar) guard plus a regression test that mounts routers cyclically?
|
@tknickman Does Vercel need the keys from strings and regexes? What's the MVP needed? Asking because it feels like the MVP to ship could be an array with no recursion (exposing For the original PR use-case of a flattened list that could be easily consumed, that is the type of thing that can be added and exported from the main package once this API is stable, since it's just a more user-friendly (but specific) output. |
|
@blakeembrey we just need the paths themselves, and ideally the methods. Eg. |
For the context of the keys #174 (comment) |
|
@bjohansebas I get that, but it's something that can be added in another PR without a breaking change. It seems like there's the bones of an implementation that can be shipped today and iterated on without any breaking changes if you define the basic API clearly. |
There is a real need to be able to list routes correctly, as clearly shown in issue (AlbertoFdzM/express-list-endpoints#99, expressjs/express#5961, expressjs/express#5858, expressjs/express#6481, https://github.com/wesleytodd/express-openapi/blob/main/lib/generate-doc.js#L16-L74, #122).
Additionally, I’ve been chatting with a Vercel employee who wants to make it possible to deploy Express servers on Vercel without any special configuration, the only blocker for them so far is that they can’t retrieve a list of the application's routes.
Previously, before the upgrade to
path-to-regexp@8(#117), it was possible to access the regex used for routing. However, this was replaced by thematcherproperty, which returns a function that extracts the path and parameters. This approach adds extra processing overhead, as it transforms a regex into a function—something unnecessary for such a simple goal. That said, it could still serve as an option for those attempting to list routes.Currently, route layers have a
pathproperty, but it's initialized asundefinedbecause the actual path is only resolved at runtime. This is due to the fact thatreq.url.pathnameisn't defined until the request is received. Once the request arrives and the pathname is parsed (as shown inrouter/index.js
Line 237 in 3d574ed
pathproperty in the layer is then set with the correct value.Proposed Solution
The solution would be to add a new property to the layer that gets initialized at the time the layer is created, using the path it was registered with. This would allow us to reliably retrieve the original path and make it possible for external tools or modules to list routes correctly.
Additionally, I'm proposing a new function that exposes the current stack's route definitions. This would reduce the need for third-party dependencies,or at least make it easier for those dependencies to support the new version of Express without relying on brittle workarounds.
Examples:
If this were to be released, in Express the function would be used like this:
Why not reverse-engineer the path?
In Express 4, there were some hacks based on inspecting the generated regular expressions. But these approaches aren't solid or future-proof. If
path-to-regexpchanges the regex structure, those solutions break. So it's not a sustainable or effective approach.Furthermore, there are plans to reintroduce some features to
path-to-regexp(pillarjs/path-to-regexp#380, pillarjs/path-to-regexp#379), so it's highly likely that the internal regex structure will change again. On top of that, unlike in Express 4, the generated regex is no longer directly accessible.close expressjs/express#5961, close expressjs/express#6481, close #122).