Skip to content

Commit

Permalink
Make sure to put the slash in for request with query strings too.
Browse files Browse the repository at this point in the history
  • Loading branch information
@creationix
creationix committed Jun 30, 2010
1 parent 4ed046a commit 42ad950
Showing 1 changed file with 5 additions and 1 deletion.
6 changes: 5 additions & 1 deletion lib/connect/index.js
View file
Expand Up @@ -239,7 +239,11 @@ Server.prototype.handle = function(req, res, outerNext) {
try {
// Trim off the part of the url that matches the route
removed = layer.route;
req.url = req.url.substr(removed.length) || '/';
req.url = req.url.substr(removed.length);
// urls should always start with a slash
if (req.url[0] !== "/") {
req.url = "/" + req.url;
}
var arity = layer.handle.length;
if (err) {
if (arity === 4) {
Expand Down

6 comments on commit 42ad950

@tj
Copy link
Member

@tj tj commented on 42ad950 Jul 1, 2010

Choose a reason for hiding this comment

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

whats the motivation behind this? breaks GET /products.json when /products is the route for router

@tj
Copy link
Member

@tj tj commented on 42ad950 Jul 1, 2010

Choose a reason for hiding this comment

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

ill see if a patch to router can deal with it

@tj
Copy link
Member

@tj tj commented on 42ad950 Jul 1, 2010

Choose a reason for hiding this comment

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

nvm got it, we just have to have the route of /.:format now instead of .:format

@creationix
Copy link
Member Author

Choose a reason for hiding this comment

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

For consistency, before it would convert empty paths to "/", but not paths like "?foo=bar" since they weren't technically empty. I looked at the failing test on router yesterday and couldn't figure out what's wrong.

@tj
Copy link
Member

@tj tj commented on 42ad950 Jul 1, 2010

Choose a reason for hiding this comment

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

async assertion failures suck unless you know the suite well

@creationix
Copy link
Member Author

Choose a reason for hiding this comment

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

yeah, I just searched for the string it was expecting and found the test case.

Please sign in to comment.