kieran / barista

A URL router / generator for NodeJS
https://kieran.github.io/barista/
MIT License
111 stars 22 forks source link

Error in barista 0.10 #16

Closed ben-ng closed 11 years ago

ben-ng commented 11 years ago

Here is the stack trace of the error when trying to access a static file in Geddy:

TypeError: Object /staticfile.css has no method 'test'
at String.exports.Route.Route.parse (/Users/ben/Documents/geddy/node_modules/barista/lib/route.js:239:17)
at Router.exports.Router.Router.all (/Users/ben/Documents/geddy/node_modules/barista/lib/router.js:82:30)
at handleNoMatchedRoute (/Users/ben/Documents/geddy/lib/app/index.js:326:39)
at /Users/ben/Documents/geddy/lib/app/index.js:450:16
at b (domain.js:183:18)
at Domain.run (domain.js:123:23)
at Server. (/Users/ben/Documents/geddy/lib/app/index.js:398:11)
at Manager.handleRequest (/Users/ben/Documents/Toolkitt/node_modules/socket.io/lib/manager.js:565:28)
at Server. (/Users/ben/Documents/Toolkitt/node_modules/socket.io/lib/manager.js:119:10)
at Server.EventEmitter.emit (events.js:98:17)
rtgibbons commented 11 years ago

Seeing the same issues using Geddy 0.9.10

techwraith commented 11 years ago

Yeah, this is holding up a deploy for at least one project that I know of. Any chance we could see a fix soon?

larzconwell commented 11 years ago

So it looks like at lib/router.coffee:135 Barista is calling apply with a string as the thisArg so the .test method is undefined.

rtgibbons commented 11 years ago

Found the issue, not sure what the fix is.

In the router.js, the all method

    Router.prototype.all = function(path, method) {
      var params, ret, route, _i, _len, _ref;

      ret = [];
      params = false;
      _ref = this.routes;
      for (_i = 0, _len = _ref.length; _i < _len; _i++) {
        route = _ref[_i];
        params = route.parse.apply(path, arguments);
        if (params) {
          ret.push(params);
        }
      }
      return ret;
    };

The route.parse.apply is getting passed in path, not route. The coffee script here is

  all: ( path, method )->
    ret = []
    params = false

    for route in @routes
      params = route.parse.apply path, arguments
      if params
        ret.push params
    ret

So my guess is the route.parse.apply path, arguments should be route.parse.apply route, argument

I'm forking and sending a fix in shortly.

techwraith commented 11 years ago

@rtgibbons Does this work for you locally?

rtgibbons commented 11 years ago

Yeah, just said that in the PR. You can make the change in the node_modules in router.js on line 82

rtgibbons commented 11 years ago

@Techwraith just update the package.json to use my commit, the issue is fixed with that change

@larzconwell does your fix it also?

larzconwell commented 11 years ago

@rtgibbons Yeah it does, I didn't notice you had already made a PR!

rtgibbons commented 11 years ago

@larzconwell I like yours better; please keep it, I'm about to remove mine.

larzconwell commented 11 years ago

@rtgibbons Alright will do

rtgibbons commented 11 years ago

FYI, this should of been a minor or major release not a patch version

http://semver.org

kieran commented 11 years ago

Holy crap, sorry guys. Will be at a computer to push a new release in about 20 min

rtgibbons commented 11 years ago

For those that are being held up by a release, run npm shrinkwrap and then go in and edit to so barista is locked in at 0.0.9

Then remove the node_modules (or the directory with barista in there). And then run npm install. It will use the old version at this point

rtgibbons commented 11 years ago

Ha, or just wait 20 minutes for @kieran to push an update :)

larzconwell commented 11 years ago

@kieran It's cool thanks!

kieran commented 11 years ago

ok, re-published v0.0.10 with the offending commit reverted, you should be good to deploy now.

Just realized there's a PR that does exactly that and adds test coverage, but github can't merge it for some reason. Will try to do it on the command line.

@rtgibbons Thanks for the reminder re: semver. I didn't mean for there to be an API change, so should this technically not be the correct level of versioning? I'm newish to semver-ing.

rtgibbons commented 11 years ago

I would have bumped a minor version on this. The idea is

MAJOR version when you make incompatible API changes, MINOR version when you add functionality in a backwards-compatible manner, and PATCH version when you make backwards-compatible bug fixes.

I would of allied this a functionality change in a backwards-compatible manner. The API didn't change, but it wasn't bug fixes either; it was a re-write of the entire code base in a backwards compatible manner.

Thanks for getting this taken care of. We all appreciate it.

kieran commented 11 years ago

Looks like I've been fucking this up on most releases! Thanks :-)