Why is `this.path` set to undefined instead of `path`?

[IamLizu]

Can someone please help me understand why this.path is set to undefined? https://github.com/pillarjs/router/blob/2e7fb67ad1b0c1cd2d9eb35c2244439c5c57891a/lib/layer.js#L43

I had checked the router Layer, and the path was undefined in almost every route, that shouldn't be the case, right?

While I don't know the reason of it being set to undefined initially, I can suggest it should be set to path. I have noticed that the path on Layer is no longer undefined if we set,

this.path = path;

Instead, it gets correct path such as /users or whatever the path actually is.

[IamLizu]

Related https://github.com/expressjs/express/discussions/5961

[carpasse]

It seems that this.path is being set to undefined because we can't determine what path the Layer is matched against until runtime.

Let me explain my thought process: Layers are added to the Router instance when the use and route methods are executed.

The Router.prototype.run method only accepts a string as path, but Router.prototype.use can accept either a string or an array of strings. You can refer to the Express documentation for more details.

We can't know which of the possible path values was matched until the layer is actually matched at runtime, as seen here.

To address the issue discussed in express#5961, perhaps we could add an extra property to the Layer instance, such as paths or pathPatterns. This would make it possible to list all routes or a router.

@pillarjs/express-tc, can we get your input on this matter?

[IamLizu]

Thank you for the explanation @carpasse.

I still have some confusion, for example, the actual matching against a request's path happens at runtime. However, if we set the Layer's path property when the Layer is created, will it create any issue?

Edit: Since the path is from request is being passed to matchLayer which is checking and setting the path to Layer in runtime, it might add a little cost but perhaps its negligible? On the other hand, instead of adding a new property, maybe we could set path during Layer creation which helps debugging.

PS: I really have very little idea, I am trying to get a better understanding.

Edit: I wonder whether any other part depends on this.path being undefined. I will try to dig in.

[carpasse]

@IamLizu I believe the path argument passed to the Layer constructor represents either a list of handler paths (as an array) or a single string. To improve clarity, It may be best to rename that constructor parameter to something like pathPatterns or layerPaths, as I think it better reflects its purpose.

• Reference 1 - Layer constructor
• Reference 2 - Layer instance creation

Additionally, the path property is expected to hold the matched portion of the runtime path based on the patterns passed to the constructor.

• This path is a substring match of the runtime path, not directly one of the paths passed to the constructor. Reference 3
• The path property cannot be defined until it has been matched against a runtime path. Reference 4
• After the match, it is effectively defined as layerPath, and then used to trim off the matched portion from the runtime path. Reference 5

[carpasse]

cc @wesleytodd as repo captain

[wesleytodd]

Yep pretty much what @carpasse has said. One thing I might ask is "what is the goal of the question"? Did you have an outcome you are looking to achieve, or is this more about exploration and learning? If the goal is listing all the routes (as is referenced near the top) then there are a bunch of discussions on that that might provide more context. But before spending time (I still have nearly 300 issues I am trying to catch up on), can I ask for a more clear explanation of the goal?

[IamLizu]

Thanks @carpasse for the explanation. I will read thoroughly and get back you if I still have questions.

@wesleytodd actually the outcome is nothing else than listing the routes. It appears setting the path to this.path instead of undefined helps easily listing the routes.

[mxngls]

I wanted to chime in on the discussion to provide a bit more context on why something proposed by @IamLizu might be needed. Wesley's package express-openapi seems to work properly for express version 4, but not version 5. As far as I can tell mostly for the same reasons @carpasse explained above (though the first error I received while trying to use the package myself with an express app using version 5, was actually a complaint about the undefined fast_slash property).

I tried to reverse-engineer the package and fix the issue myself. It actually works pretty well, except for the fact that there is no way of obtaining paths for registered middleware (nested routers for instance!).

To come back to Wesley's original question, the goal of adding some kind of static path property would be to allow to generate OpenAPI docs in a reliable way. The actual grunt work that might be necessary to get rid of wildcards or other things mangling the path string(s) in a way that's incompatible with the OpenAPI spec might then simply be left to the user. This, at least in theory, doesn't create much additional work to maintain and provides users with an ergonomic way of obtaining paths for routers.

[wesleytodd]

Ah yes, one of my goals over the next weeks is to update that open api package. I was just slammed with other things through the end of last year. @mxngls if you are interested in helping with that work I might be able to give a few pointers. Honestly I think express/router itself should have some better apis for this kind of thing so that third parties dont have so much work to do for compatibility in as we make future changes. So might be we could work on landing that kind of stuff here as well to help.

[mxngls]

Thank you for your swift response @wesleytodd. Actually I already came up with a solution, that works quite well. I might have time to publish the refactored package over the weekend. I think to get rid of regular expressions for paths was a good decision overall. The work that was necessary to correctly parse these paths with regular expressions seems incredible convoluted and quite error prone.

I intend to make the first version only compatible to Express version 5 and upwards for the same reasons. What is still left to do is to transform the paths as used by Express to something that is accurate to the OpenAPI specification. I don't think this will prove to be too cumbersome, as we only need to adjust the syntax for regular and optional parameters. Wildcard parameters are not conform to the spec anyway. I would love to ping you again and if possible get some input from your side. The package itself is written in Typescript and considerable leaner than the original express-openapi package written in Javascript. But I think it would be better to move that talk into a separate discussion as it's not really an issue.

Lastly I fully agree regarding your comments on adding some proper APIs to Express so that obtaining router paths is not as arduous as it is right now.

[hanse]

This is kind of blocking us from upgrading from express v4 and express v5 as we rely on being able to infer the full list of routes for e.g. populating a openapi spec or listing all available endpoints. Our express v4 solution works perfectly well, but it does not work fully in express v5 and it seems partly due to internal changes in router making in impossible(?) to infer full paths for nested routers.

router = Router()
router.get("/b", () => {})

app.use("/a", router)

In express v4, our route listing mechanism could easily display like:

GET /a/b

[Speidy674]

when this.path is designed to hold the match item why not just add a variable that hold the defiend patterns and ppl can get all possible routes again

this.pathPatterns = path

[mxngls]

@wesleytodd Took me way too long to finally get this on GitHub but here we are: express-openapi (I decided to keep the name as the old version for now). We've been using this in production for a while now and it works pretty well. I am sure it has some rough edges and lots of stuff could be improved, but the basic OpenAPI integration works pretty decently.