bminer / node-static-asset

Static asset manager for Node.JS and Express
MIT License
48 stars 13 forks source link

Switch req.url to req.originalUrl for consistency with serve-static module #16

Closed bkniffler closed 9 years ago

bminer commented 9 years ago

Thanks for the PR. I think that I understand this request, but could elaborate on the use case here?

bkniffler commented 9 years ago

I think the title is misleading, I was in a bit of a rush when I posted this.

According to http://expressjs.com/api.html#req.originalUrl, req.originalUrl exists so you can modify req.url for internal routing. So if you want the unmodified url (which is what we should want here), you should rely on originalUrl.

In my case, req.url differs from req.originalUrl (e.g. "/?v=194m-8bn6a2" vs "/style.css?v=194m-8bn6a2"), though I'm not completely sure which middleware is responsible. I'll look into that later.

This is how i currently workaround the issue

app.use(url + "*", function(req, res, next){
        var url = req.url;
        console.log(req.url, req.originalUrl);
        req.url = req.originalUrl;
        staticAsset(path.join(dir, 'assets'))(req, res, function(){
            req.url = url;
            next();
        });
    });
bminer commented 9 years ago

I gotcha, but why not put the node-static-asset middleware before your internal routing? Again, just trying to understand the use case before merging.

Thanks again.

bkniffler commented 9 years ago

I'm using strongloops loopback as an extra layer, which is build on top of express. As it looks, its messing up the req.url, though I'll have to dive deeper into the code. But unfortunately, loopback is not simply a middleware.

Throughout my research, I've been looking into the serve-static module by express (https://github.com/expressjs/serve-static/blob/master/index.js) and I noticed they're using originalUrl too.

bminer commented 9 years ago

Good enough for me. Merging into master...