visionmedia / express-resource

Resourceful routing for Express
1.41k stars 140 forks source link

Ordering bug #35

Closed tj closed 13 years ago

tj commented 13 years ago

because of Object.keys() exporting .new etc are not order independent, this is definitely a bug

jupiter commented 13 years ago

This was fixed in https://github.com/visionmedia/express-resource/pull/21

jupiter commented 13 years ago

Also, when you do a custom action mapping on a resource with a show action, the show action would always be a preferred route. E.g. from tests:

    function load(id, fn) { fn(null, "User"); }
    var actions = {
      show: function(req, res){
        res.end('user');
      },
      login: function(req, res){
        res.end('login');
      },
      logout: function(req, res){
        res.end('logout');
      }
    };

    var users = app.resource('users', actions, { load: load });
    users.map('get', 'login', actions.login);
    users.map('get', '/logout', actions.logout);

Which, when running the test, results in:

   uncaught: AssertionError: test custom route configuration. Invalid response body.
    Expected: 'logout'
    Got: 'user'
tj commented 13 years ago

fixed by e4e70805e09a0cf43b3edeb45a4d123a20fb2825

brynbellomy commented 12 years ago

In response to jupiter's comment that custom routes can never be called, I've found a way to do it, albeit one that feels pretty hackish.

var theResource = app.resource('users'); // note that the second argument is left unspecified
theResource.map('get', '/login', UserController.getLogin);
theResource.map('post', '/login', UserController.postLogin);
theResource.map('get', '/logout', UserController.getLogout);
theResource = app.resource('users', UserController); // ... and here, the second argument is specified as usual

Given that it's not impossible to make this work, I don't think it's worth pushing for any kind of change in the code, at least for the time being. I'm really just providing this workaround for others who've encountered the same confusing bug. Maybe worth changing the one test that fails, though, since people treat tests as documentation.