josephwegner / simple-api

A Node.js API Scaffolding Module
MIT License
83 stars 10 forks source link

General concerns #8

Open gr0uch opened 11 years ago

gr0uch commented 11 years ago

Hi, I'd like to address a few concerns about this package:

P.S. I've authored a package that may be similar.

josephwegner commented 11 years ago

Hi Dali,

Before I respond to each of your points individually, I think there is a misunderstanding about the relationship between routes and actions that needs to be cleared up.

Route objects and action objects should be named the same - that is how the action is chosen. Under the hood, Simple API loops through each of the routes - the first things it checks is if there is an action defined with the same name as the key of the route. Then it does the path matching and parameter pulling, and passes all that to the matched action.

Now specific responses:

  1. This was a very intentional decision. Other API frameworks (namely Rails) use simple action names (ie: PUT => update, POST=> create). Those work fine (not great) for top level actions, but nested actions are quite confusing. User-defined names make code much more readable, and do not require any future maintainers to understand Simple or common HTTP method patterns.
  2. You're correct, those patterns are just for type casting. Admittedly, they add a significant layer of confusion, and I've considered removing them. I like them because you can define actions like getUserById and getUserByName with the same path, buy only the format of the identifier changes. I will consider if there is a better way to so this, or if I should just remove it completely.
  3. I entirely agree. As I was upgrading all of my systems to v0.1 I kept thinking to myself "why the heck did you make this synchronous?!?!". This will be changed in the next major release.
  4. I had considered an after hook, and then decided there was no need because of the convenience response functions. Simple does not do any fancy stuff after your controller actions are called, so any post processing you want done can be easily manually defined or just overridden in the response functions. That said, I'm not closed to the idea of an after hook - could you provide some use cases so that I can make sure I architect it in the most useful way?
  5. This is explained in the leading paragraph. I would imagine your confusion comes from poorly written docs - I will update them to be more useful.
  6. Helpers are purely for organization. The concept came from my PHP days with Zend. When learning Zend I often asked myself, "where is the right place to put this function?". Helpers were the solution there, and I think that's a great pattern. You can define helper functions anywhere - they don't need to be scoped into the controller - but I think having a defined place will keep controller formats consistent.

Also, Fortune looks great! Seems like a more poweful version of Deployd.

I have an eventual feature planned to write common top level controller actions (PUT, POST, etc). That will require model integration, which is something I'm thinking very hard about before I actually build it. I want my models to be DB agnostic - I haven't found the right way to do that, though.

By the way, I'm going to do a more formal review of these items and then break them out into individual issues. That fits better with my ( and OS in general) development processes. I'll copy you on each of the issues so you get notified.

Thanks! On Sep 10, 2013 1:05 AM, "Dali Zheng" notifications@github.com wrote:

Hi, I'd like to address a few concerns about this package:

  • The key names in the routes object seem redundant. This is mostly an aesthetic concern, but it does force the user to think of a name for a route, even if the route is self-descriptive.
  • Not sure about the purpose of differentiating :, %, or * parameters, other than to have some sort of type casting. After all, the URL is just a string.
  • Making the before method synchronous really limits what you can do with it. For example, what if I want to query the database to check if the currently authenticated user has permissions on a resource? Consider returning a promise or callback.
  • Come to think of it, it might be missing an after method, which gets called after a resource is loaded, to transform the response.
  • It's unclear how a route triggers an action. How does a route know what is the corresponding action?
  • Is there any point to the helpers object, or is it there for organizational purposes?

P.S. I've authored a package that may be similarhttps://github.com/daliwali/fortune.

— Reply to this email directly or view it on GitHubhttps://github.com/josephwegner/simple-api/issues/8 .

gr0uch commented 11 years ago

Ah, your explanation of the routes clears things up.

Just an example of how an after hook might be useful: setting the Content-Type on every response, without having to define this on each controller, or formatting the output in a certain way (no spaces for production, 2 space indent for dev).

I want my models to be DB agnostic - I haven't found the right way to do that, though.

This is something I've done with Fortune: define models once, use any adapter. Models are wrapped in a resource object, and the wrapper contains common methods to act on a model (find, create, update, delete).