Add barista #184

Merged
merged 4 commits into from Sep 5, 2012

Projects

None yet

4 participants

Contributor
kieran commented Sep 5, 2012

Switched to Barista, added 405 error, fixed a static file bug, updated router template

@mde mde merged commit 03b3aac into geddy:master Sep 5, 2012
Contributor
mde commented Sep 5, 2012

This is fantastic. :)

Contributor

Woah awesome!

Contributor
polotek commented Sep 16, 2012

This patch fixes @kr1zmo's problem above. But it does raise an issue with barista. The barista Router is not fully compatible with the old geddy router. It wasn't caught because the tests weren't updated to reference barista. Apply this patch and you'll see.

diff --git a/test/routers/regexp_router.js b/test/routers/regexp_router.js
index 325bc9e..7ee705d 100644
--- a/test/routers/regexp_router.js
+++ b/test/routers/regexp_router.js
@@ -1,7 +1,7 @@
 // Load the basic Geddy toolkit
 require('../../lib/geddy');

-var Router = require('../../lib/routers/regexp_router').RegExpRouter
+var Router = require('barista').Router
   , assert = require('assert')
   , tests;

These compatibility problems seem fairly shallow and can be worked around. It's just a question of how we want to do that. If we don't want to break the geddy api, I suppose we'll have to have a light shim over barista.

Contributor
mde commented Sep 16, 2012

Ah, of course, we're just relying Barista's tests, and there's nothing to ensure backward-compat. I guess we need a shim, with our own tests for the specific API calls we care about. I don't have a strong objection to deprecating the old API calls over time either, at a major release point.

Contributor
mde commented Sep 16, 2012

Okay, I've done a little bit of cleanup that fixes issues with controller-instantiation and adding routes to the config in an app's router.js. Things should be working as they were before.

A few items of note:

  1. As @kr1zmo noted, controller-names returned by Barista are in snake-case, but Geddy expects them in "constructor" style -- camel-case with initial cap. Since we're in JavaScript land, and variables are never expected to be in snake-case, I think the Geddy way makes more sense inside the app. We'll continue to do things that way in Geddy, and simply rewrite params.controller to be what Geddy expects.
  2. There were no real regressions in the tests, other than the casing-change in the controller names. I'm just going to remove the old router tests, and as Marco notes, when we begin pulling apart the app-loading code in app.js, we should be adding integration tests there.
  3. HEAD requests in Barista looks to be implicitly handled for all GET routes, which is probably a good thing -- but it is a change. The only place I can imagine this might have an impact is in the list of acceptable methods returned in a 405, but I'm not really sure how much I care about that.
  4. Barista is using the NPM 'inflections' module for inflections, and we're using our own version in the 'utilities' lib. We should probably converge on using the NPM module, if it does everything we want, just to make sure we don't run into subtle inconsistencies between the two.
Contributor
polotek commented Sep 16, 2012

Are you sure you want to invalidate the current router tests? They do a lot
more to show incompatibility. The barista router creates less routes for
one thing. I was going to dig in. But when you call resource in the old
router you got 11 routes. With barista, you get 7. We should probably
document the differences and be explicitly aware of what the changes are.

On Sun, Sep 16, 2012 at 2:33 PM, Matthew Eernisse
notifications@github.comwrote:

Okay, I've done a little bit of cleanup that fixes issues with
controller-instantiation and adding routes to the config in an app's
router.js. Things should be working as they were before.

A few items of note:

As @kr1zmo https://github.com/kr1zmo noted, controller-names
returned by Barista are in snake-case, but Geddy expects them in
"constructor" style -- camel-case with initial cap. Since we're in
JavaScript land, and variables are never expected to be in snake-case, I
think the Geddy way makes more sense inside the app. We'll continue to do
things that way in Geddy, and simply rewrite params.controller to be what
Geddy expects.
2.

There were no real regressions in the tests, other than the
casing-change in the controller names. I'm just going to remove the old
router tests, and as Marco notes, when we begin pulling apart the
app-loading code in app.js, we should be adding integration tests there.
3.

HEAD requests in Barista looks to be implicitly handled for all GET
routes, which is probably a good thing -- but it is a change. The only
place I can imagine this might have an impact is in the list of acceptable
methods returned in a 405, but I'm not really sure how much I care about
that.


Reply to this email directly or view it on GitHubhttps://github.com/mde/geddy/pull/184#issuecomment-8599898.

Marco Rogers
marco.rogers@gmail.com | https://twitter.com/polotek

Life is ten percent what happens to you and ninety percent how you respond
to it.

  • Lou Holtz
Contributor
polotek commented Sep 16, 2012

Oh I see. The difference in number of routes is explained by 3 above. As
long as you can explicitly override and get separate HEAD routes if you
need them, I think that's okay.

On Sun, Sep 16, 2012 at 2:40 PM, Marco Rogers marco.rogers@gmail.comwrote:

Are you sure you want to invalidate the current router tests? They do a
lot more to show incompatibility. The barista router creates less routes
for one thing. I was going to dig in. But when you call resource in the old
router you got 11 routes. With barista, you get 7. We should probably
document the differences and be explicitly aware of what the changes are.

On Sun, Sep 16, 2012 at 2:33 PM, Matthew Eernisse <
notifications@github.com> wrote:

Okay, I've done a little bit of cleanup that fixes issues with
controller-instantiation and adding routes to the config in an app's
router.js. Things should be working as they were before.

A few items of note:

As @kr1zmo https://github.com/kr1zmo noted, controller-names
returned by Barista are in snake-case, but Geddy expects them in
"constructor" style -- camel-case with initial cap. Since we're in
JavaScript land, and variables are never expected to be in snake-case, I
think the Geddy way makes more sense inside the app. We'll continue to do
things that way in Geddy, and simply rewrite params.controller to be what
Geddy expects.
2.

There were no real regressions in the tests, other than the
casing-change in the controller names. I'm just going to remove the old
router tests, and as Marco notes, when we begin pulling apart the
app-loading code in app.js, we should be adding integration tests there.
3.

HEAD requests in Barista looks to be implicitly handled for all GET
routes, which is probably a good thing -- but it is a change. The only
place I can imagine this might have an impact is in the list of acceptable
methods returned in a 405, but I'm not really sure how much I care about
that.


Reply to this email directly or view it on GitHubhttps://github.com/mde/geddy/pull/184#issuecomment-8599898.

Marco Rogers
marco.rogers@gmail.com | https://twitter.com/polotek

Life is ten percent what happens to you and ninety percent how you respond
to it.

  • Lou Holtz

Marco Rogers
marco.rogers@gmail.com | https://twitter.com/polotek

Life is ten percent what happens to you and ninety percent how you respond
to it.

  • Lou Holtz
Contributor
mde commented Sep 16, 2012

Yeah, we really need some integration tests, but I think the current router tests just muddy the waters. app.js is kind of a nightmare. I've done some work on the init stuff over in the remove-model branch (which is sort of the de facto dev branch right now), but the request handling stuff is really fucking out of control.

Contributor
polotek commented Sep 16, 2012

I'm finding lots more differences here. What I'd like to do is take a look at the barista tests and the current geddy routes tests with a critical eye. We need to settle on an interface for geddy, and then decide how Barista fits in. Obviously I won't be able to devote time to this for a few weeks. But right now things are pretty broken, even if you're trying to follow the geddy tutorials. I don't know how the rest of you feel, but I recommend rolling this back for now. This is no slight to barista. It's just that we need to rethink this a bit.

I'll leave it up to @mde though.

Contributor
kieran commented Sep 17, 2012

IIRC, Barista returns the actual controller name un-modified, it simply snake cases (and pluralizes?) the URL portion when generating resources.

i.e. resource('BlogPost') will map to the BlogPost controller, but will generate urls in the form /blog_posts/:id

HEAD is an alias for GET, but HEAD is the method passed in with the params. As far as I can tell, this seems to be the desired behavior since HEAD requests are analogous in every way except that we don't send actual body content. I could be wrong though, it's happened at least once before ;-)

Are there any other inconsistencies? I'm all for fixing Barista if it's a router issue!

Contributor
polotek commented Sep 17, 2012

As I said, there's nothing wrong with barista. The problem here is
compatibility, and that is not something to take lightly. Geddy is aiming
to be a framework that works for projects from small to large. And telling
people with large projects to "just change your routes" is doing a
disservice those people looking for stability and consistency.

Also the compatibility issues aren't all minor. It looks like barista is
lacking some features present in the old routing system. The old routes
take an options object where you can do things like add validation to route
parameters. Unless I'm missing something, barista does not. It may be that
nobody's using that feature, which is fine. Although I haven't looked at it
in depth, barista looks like a nice implementation. I wouldn't mind having
it as the default implementation for geddy and serve as a "baseline"
feature set. But we do have to make some hard decisions about the features
that are going away.

Another thing that I think it is important, is keeping the major components
of Geddy behind solid interfaces that can be relied upon. What this means
is that whatever public api we decide on for routes needs to be supported
even if we swap out (or enhance) the underlying implementation. That should
include inputs, outputs and method interfaces. This is not that difficult
to do. And the upside is that the tests for geddy stay valid and always
test intended behavior instead of testing implementation. Because as we saw
here, that is easy to break if you're not careful. The rails community
learned this less as well and Rails 3 is much more modular underneath.

For instance, the test for router should say
require('../lib/geddy').Router. Even if that file is this.
module.exports = require('barista');.

I've been a committer for a while, but just really getting a handle on the
internals of geddy. I don't want to be a barrier to progress or anything,
and I've talked with the other folks about how geddy should be using
existing modules instead of writing our own. But I am also concerned with
consistency in the api. Geddy is still growing, and not yet 1.0.0. So
there's room to move here. But let's move deliberately.

:Marco

On Sun, Sep 16, 2012 at 6:52 PM, Kieran Huggins notifications@github.comwrote:

IIRC, Barista returns the actual controller name un-modified, it simply
snake cases (and pluralizes?) the URL portion when generating resources.

i.e. resource('BlogPost') will map to the BlogPost controller, but will
generate urls in the form /blog_posts/:id

HEAD is an alias for GET, but HEAD is the method passed in with the
params. As far as I can tell, this seems to be the desired behavior since
HEAD requests are analogous in every way except that we don't send actual
body content. I could be wrong though, it's happened at least once before
;-)

Are there any other inconsistencies? I'm all for fixing Barista if it's a
router issue!


Reply to this email directly or view it on GitHubhttps://github.com/mde/geddy/pull/184#issuecomment-8602780.

Marco Rogers
marco.rogers@gmail.com | https://twitter.com/polotek

Life is ten percent what happens to you and ninety percent how you respond
to it.

  • Lou Holtz
Contributor
kieran commented Sep 17, 2012

I totally agree that having a solid API is necessary.

Not sure how the API is different now than it was before, though I have added a few features like nested routes & resources. Do you happen to remember any of the differences off-hand?

Maybe I should start adding features to a 0.1.x branch, then stabilize them in a 0.2.x branch with bug fixes. That way Geddy can safely include 0.0.x, then update to the 0.2.x line when it's ready.

re: validating params, do you mean this? http://kieran.github.com/barista/#conditions It's been there (and un-touched) since the early days. Maybe it got dropped from the Geddy docs?

I really do have to finish updating the docs for Barista, especially the sections on new features. It's open in a text editor as I write, but I've been on vacation this week. Also: I'm a little lazy when it comes to docs. I'll slap my hands when I get back next week & push to gh.

Contributor
polotek commented Sep 17, 2012

Yeah I think this feature is equivalent. But the api is different. I'm
going by the geddy tests. See
https://github.com/mde/geddy/blob/master/test/routers/regexp_router.js#L72.
It may be that the differences are only in the api.

:Marco

On Sun, Sep 16, 2012 at 7:11 PM, Kieran Huggins notifications@github.comwrote:

I totally agree that having a solid API is necessary.

Not sure how the API is different now than it was before, though I have
added a few features like nested routes & resources. Do you happen to
remember any of the differences off-hand?

Maybe I should start adding features to a 0.1.x branch, then stabilize
them in a 0.2.x branch with bug fixes. That way Geddy can safely include
0.0.x, then update to the 0.2.x line when it's ready.

re: validating params, do you mean this?
http://kieran.github.com/barista/#conditions It's been there (and
un-touched) since the early days. Maybe it got dropped from the Geddy docs?

I really do have to finish updating the docs for Barista, especially the
sections on new features. It's open in a text editor as I write, but I've
been on vacation this week. Also: I'm a little lazy when it comes to docs.
I'll slap my hands when I get back next week & push to gh.


Reply to this email directly or view it on GitHubhttps://github.com/mde/geddy/pull/184#issuecomment-8602940.

Marco Rogers
marco.rogers@gmail.com | https://twitter.com/polotek

Life is ten percent what happens to you and ninety percent how you respond
to it.

  • Lou Holtz
Contributor
mde commented Sep 17, 2012

@kieran Right, it just returns the thing unmodified. I want to depend on using it as the constructor name, so I'm just rewriting params.controller to make sure it's constructor-correct. This was the only real incompatibility as far as I see, and did actually affect the normal workflow of creating resource-routes -- the "frang_zooby" resource should yield a "FrangZoobies" controller.

(There was also a small issue with the changes to the router template, because the ugly code that has to open it up and add the routes depends on the "module.exports" line to be at the bottom, but that was a simple fix.)

I seriously doubt anyone was using any of the more advanced features of the router yet, especially since none of them were documented or publicized. We do need to put our big-boy pants on, and protect backward-compat more going forward, but I genuinely don't think this will be a major issue at this point. I saw we rip this bandaid off.

Here's what docs we have:

https://github.com/mde/geddy/wiki/Using-the-Router

@kieran, I guess you wrote a lot of that. Would it be possible to make sure it's up to date with current reality? Of course, a link pointing to the (soon to be, cough) copious docs for Barista would be awesome for people want to learn the fancier features.

@polotek has a really valid point about interfaces. What do people think about specifying an exact version-number of Barista in package.json? Or do we basically expect a guarantee from @kieran that APIs never change within a major version?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment