SF-WDI-LABS / shared_modules

SF WDI shared lecture notes
9 stars 59 forks source link

express w/ or without controllers #71

Closed tgaff closed 8 years ago

tgaff commented 8 years ago

We previously looked at 26's code and thought it was better organized. They have controllers for each route and use express.Router. We also considered that this might be overly complicated especially on the first day.

Please discuss, should we:

  1. Stick with our traditional approach of all the routes in server.js. See also this lab solution
  2. Move immediately to putting each route in controllers and using express.Router see an example of that here. :warning: Note that this requires re-writing pretty much all of our Node stuff.
  3. Delay this and start with the traditional approach, later (maybe the next day with Todo Lab, or the next week with Tunely) switch to Router and controllers.
  4. Something even better that you'll explain to us here. :grey_question:
  5. Juliana suggested what I'm calling a namespace-less mountable app (no router) She also suggested calling the files resources instead of controllers:
module.exports = function(app) {
​
  app.get('/api/admin/unconfirmed-users', auth.ensureAuthenticated, auth.ensureAdmin, function (req, res) {
    User.find({ confirmedAt: null }, '+email').exec(function (err, users) {
      res.send(users)
    })
  })
}

Please note that this issue is time-sensitive. Next week's lessons are already due and last-minute changes are bad news bears. :bear:

tgaff commented 8 years ago

I think we said we'd go with 3, but I'm not sure we got consensus there.

jlopker commented 8 years ago

i think i'm leaning towards 2. it's not a huge amount of more files and i would rather not have to deal with the confusion later. i also think that this pattern would help with understanding MVC. but i'm willing to be convinced of something else.

tgaff commented 8 years ago

Nathan said:

" One option might be to separate out controllers starting with MEAN stack. And use inline functions from day one so they get familiar with naming conventions: eg todoShow, todoIndex, etc. We should consider stubbing empty controllers, a la rails, especially because of export/import. Plus it would enforce REST pattern. "

tgaff commented 8 years ago

A loose lunchtime poll showed that: Justin: is somewhat on the fence but leaning toward 3 (delay and ask Nathan to do it the next day) Cory: Willing to let us choose. Brianna: Very much in favor of the Router and controllers. Juliana: Doesn't like router but likes the idea of controllers in separate files. (Note: maybe I misunderstood since this conflicts a little with @jlopker above)

tgaff commented 8 years ago

I've added an example to Tuesday's TodoApp. https://github.com/SF-WDI-LABS/test-driven-todo-api/blob/use_separate_controllers_no_router/server.js This:

@jlopker is this what you meant?

tgaff commented 8 years ago

5: Juliana suggested what I'm calling a namespace-less mountable app (no router) She also suggested calling the files resources instead of controllers:

module.exports = function(app) {
​
  app.get('/api/admin/unconfirmed-users', auth.ensureAuthenticated, auth.ensureAdmin, function (req, res) {
    User.find({ confirmedAt: null }, '+email').exec(function (err, users) {
      res.send(users)
    })
  })
}
tgaff commented 8 years ago

OK. Nathan is out on Monday and unavailable for comment. If we changed Monday's lesson, that would force @cofauver to change Tuesday's and then Nathan would arrive Tuesday morning to find that we'd pulled the rug out from under him. I would hate to have that happen to me and don't want to do it to him. Plus I think Cory is still getting up to speed a little bit and asking him to change it all at last minute on the weekend seems a little sad.

tgaff commented 8 years ago

We can still discuss introducing controllers and maybe Router later-on. (maybe around the time we get 2 models)

tgaff commented 8 years ago

:camel: :cactus: :cake: :ramen: :dromedary_camel: :dragon: :microphone: :100: :+1: :rabbit: :dancers: :camel: :cactus: :cake: :ramen: :dromedary_camel: :dragon: :microphone: :100: :+1: :rabbit: :dancers:

So a meeting of 4 today (BV, JL, CF, TG) determined that on Thursday afternoon the module pattern would be introduced and we'd go with:

Effectively that's the example here: https://github.com/SF-WDI-LABS/test-driven-todo-api/blob/use_separate_controllers_no_router/server.js but with a few minor changes like require each controller separately and name functions 'index', 'show', 'create' etc.

That puts this right after learning mongoose and therefore reinfoces module pattern. It also is reasonably Railsy.

tgaff commented 8 years ago

This is answered. Closing.