expressjs / vhost

virtual domain hosting
MIT License
761 stars 87 forks source link

Wildcard matching should pass on the matched subdomain name #3

Closed chucklam closed 10 years ago

chucklam commented 10 years ago

1) For wildcard matching, I refactored and added a couple more tests to ensure that matching is done in the order the vhosts are added. Nothing unexpected here. Just some clarification.

2) When using wildcard to match subdomains, I'd like to know the subdomain name that was matched. For example, I may have one server serving www.example.com while another one serving other subdomains like john.example.com, larry.example.com, and moe.example.com. In the second case I want to know whether I'm serving john, larry, or moe. I decide to return that as a string in req.vhost. In principle one can use multiple wildcards, so there can be multiple matched parts. I imagine that's a really rare use-case though, so I focused on returning just the first match and make it user-friendly for that case. I'm open to discussion though.

dougwilson commented 10 years ago

In principle one can use multiple wildcards, so there can be multiple matched parts

That needs to be supported, if you are going through the trouble to populate a req.vhost instead of simply inspecting req.headers.host in your handlers, otherwise what does this get you over just checking req.headers.host besides a half-implemented value?

chucklam commented 10 years ago

Ok, going all the way and return all matches. The additional test shows that it works with multiple wildcards.

chucklam commented 10 years ago

Now all matches are returned in req.vhost.

dougwilson commented 10 years ago

Cool. i.m.o. req.vhost[0] seems like a better API. I'm not sure if we should reverse the list or not (i.e. *.*.example.com against foo.bar.example.com should return ['bar', 'foo'] vs ['foo','bar']) such that req.vhost[0] is consistently the right-most. This reverse would make it match req.subdomains in express.

chucklam commented 10 years ago

Interesting point on reversing the list. I think if the main use case for multiple wildcards is to match multilevel subdomains, then it should be reversed to be consistent with req.subdomains. Since I can't think of any other use case, that's probably the right way to go.

chucklam commented 10 years ago

Just checking in to see if everything is approved for merging.

dougwilson commented 10 years ago

Just checking in to see if everything is approved for merging.

Thanks for checking in :) Yes, it should be. This and a couple more things I need to merge, which I plan to do after a connect/express cycle tonight :)

dougwilson commented 10 years ago

Now that that connection/express cycle is done, I'm going to merge this. Should be tonight. Just giving a heads up