Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Make req.route an object saving route and baseUrl #37

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

tunniclm
Copy link

This PR attempts to implement expressjs/express#2883.


When using an error handler you have access to req.route which describes the most recently matched route. However, some other informaiton (like req.baseUrl) gets changed before the error handler runs.

This commit changes req.route to an object that also contains the req.baseUrl before it gets changed.

Since the actual route object is not scoped to the current event, this commit instead uses a new object to store the baseUrl but sets the __proto__ to the route object so that all its properties are also available.

When using an error handler you have access to req.route
which describes the most recently matched route. However,
some other informaiton (like req.baseUrl) gets changed
before the error handler runs.

This commit changes req.route to an object that also contains
the req.baseUrl before it gets changed.

Since the actual route object is not scoped to the current
event, this commit instead uses a new object to store the
baseUrl but sets the __proto__ to the route object so that
all its properties are also available.
@tunniclm
Copy link
Author

Some thoughts/questions:

  • Is using __proto__ ok? It could instead copy the values to a new object {}, or alternatively should it be a new object with a differnet name, such as req.routeInfo, that contains properties req.routeInfo.route and req.routeInfo.baseUrl?
  • Is the test okay? Should it instead (or additonally) test the value of req.route.baseUrl in an error handler? Should it use a different style of test where the value checked is in the body of the response rather than a header?

@@ -271,7 +271,8 @@ Router.prototype.handle = function handle(req, res, callback) {

// store route for dispatch on change
if (route) {
req.route = route
req.route = req.route || { __proto__: route }
req.route.baseUrl = req.baseUrl;
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oops, must remove the habitual ;

@tunniclm
Copy link
Author

tunniclm commented Mar 8, 2016

Any thoughts/feedback on this?

@wesleytodd
Copy link
Member

Hey! I am (finally) getting around to trying to wrangle these and I think that this is a good idea. Sadly I think it is breaking and since it is so old I am not sure it is a good idea to revive this as is or take a new try specifically on top of the 2.0 branch? If you are interested in landing this for our push toward express@5 I would love to chat. If not, I think it will need to wait for v6. Let me know asap so I can work it into the plans.

# for free to join this conversation on GitHub. Already have an account? # to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants