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

Why is this.path set to undefined instead of path? #122

Open
IamLizu opened this issue Sep 25, 2024 · 7 comments · May be fixed by #123
Open

Why is this.path set to undefined instead of path? #122

IamLizu opened this issue Sep 25, 2024 · 7 comments · May be fixed by #123
Labels

Comments

@IamLizu
Copy link
Member

IamLizu commented Sep 25, 2024

Can someone please help me understand why this.path is set to undefined?

this.path = undefined

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 IamLizu linked a pull request Sep 25, 2024 that will close this issue
@IamLizu
Copy link
Member Author

IamLizu commented Sep 25, 2024

Related expressjs/express#5961

@carpasse
Copy link
Contributor

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
Copy link
Member Author

IamLizu commented Oct 5, 2024

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

carpasse commented Oct 7, 2024

@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.

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

carpasse commented Oct 7, 2024

cc @wesleytodd as repo captain

@wesleytodd
Copy link
Member

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
Copy link
Member Author

IamLizu commented Nov 23, 2024

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.

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

Successfully merging a pull request may close this issue.

3 participants