Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Replace ViewModel with MVC Style base class #13

Open
mjreich opened this issue Aug 23, 2016 · 9 comments
Open

Replace ViewModel with MVC Style base class #13

mjreich opened this issue Aug 23, 2016 · 9 comments

Comments

@mjreich
Copy link
Contributor

mjreich commented Aug 23, 2016

We do a lot of simple MVC style parts of applications. Rather than writing all the connections between models, views, routes and controller logic, it would be nice to have a helper module base class that automates/reduces the code.

import {MVCModule} from 'nxus-mvc'

class Thing extends MVCModule {
  constructor(opts) {
    super(opts)
  }

  index(req, res) {
    return {some, opts} // simple object return that is passed to the view
  }

  view(req, res) {
    return this.models.Thing.find().then((things) => {
      return {things}
    })
  }

  edit(req, res) {
    return this.models.Thing.findOne(req.params.id).then((thing) => {
      return {thing}
    })
  }
}

This would expect the following folder structure with models and templates

-index.js
-models/
  |-thing.js
-templates/
  |-index.ejs
  |-view.ejs
  |-edit.ejs

The following routes would automatically be created. The root is the lowercased, singularized constructor.name

GET /thing
GET /thing/:id
GET /thing/:id/edit

PUT /thing
POST /thing/:id
DELETE /thing/:id
@mjreich
Copy link
Contributor Author

mjreich commented Aug 23, 2016

Admin-ui should use this class and provide an admin-ui specific theme and templates for the views, but overall could use exactly the same controller/config logic.

@loppear
Copy link
Contributor

loppear commented Aug 30, 2016

👍 to a convenience base class that contains all of this, noting some thinking/discussion about more granular mixins that this MVCModule might be composed of (some of these are pretty minimal gain over just implementing in constructor, and still require coordination in the participating constructor):

  • storage.HasModels:
    • reads ./modules
    • configured by this.modelNames, modulo questions about access to other modules models.
    • accessed by this.models.modelInstance
  • templater.HasTemplates
    • templateDir on ./templates
    • may enforce some template naming prefix etc
  • clientjs.HasScripts
    • reads ./js (?)
    • or configured by includedJS to set path names and template names to set scripts.
  • router.ProvidesRoutes
    • configured by routes for mapping of routes to bound methods?

@mjreich
Copy link
Contributor Author

mjreich commented Aug 30, 2016

@loppear to borrow from Rails, the benefit of this class seems to be use of convention rather than explicit configuration. Otherwise, we’re creating two different ways of explicitly configuring templates/routes, etc (this way and the standard direct way router.route(…). So the following questions:

  1. Do we want to explicitly configure models/templates or just load in the files like we did with the old Storage autoloading of ./models?
  2. HasScripts interesting. Propose the convention would be: ./client/template-name.js -> auto injected into ./templates/template-name.ejs
    1. I’ve been using client recently instead of js since its all technically javascript (seems clearer), but open to other suggestions.
  3. ProvideRoutes should be HasRoutes to be consistent? Again, should assume a followed convention (Module.index, Module.view, Module.save, Module.edit, etc) instead of explicit configuration?

@loppear
Copy link
Contributor

loppear commented Aug 30, 2016

I think it makes sense for Storage to get a modelDir() analog to templateDir, so that's most of the way. (Old _loadLocalModels was just app-level, not module-level).

I think it makes sense to make the convention of "models in ./models, templates in ./templates" automatic if we're going to have this base class. Open question right now of whether it's even possible for the base class to find out the path to the concrete sub-class however, so maybe moot. But I'd really rather not have every sub-class of MVCModule need to state super(); templater.templateDir(__dirname+"/templates"); storage.modelDir(__dirname+"/models"), total boilerplate.

Yes to './client' for client js, and for 'scripts' or 'client' as the general term for 'js for the client side' in method names.

Yes to MVCModule making use of HasRoutes (or equiv in constructor) to specify that fixed method convention for this class.

@mjreich
Copy link
Contributor Author

mjreich commented Aug 30, 2016

👍

@loppear
Copy link
Contributor

loppear commented Aug 30, 2016

Diving into the details of implementing these route handlers, questions:

  • Template names - we'd prefer to use prefixed template names (derived from module name by default: thing-list.ejs), this is only a little clunky with providing default implementations - MVCModule would register templater.default().template('mvc-list.ejs', 'page', this.prefix()+"-list")
  • Current proposal has these view/edit/etc methods just wrapping the template rendering. Seems like they should also wrap the object retrieval (in order to support pagination at least?)

@loppear
Copy link
Contributor

loppear commented Aug 30, 2016

More details thinking out loud:

  • MVCModule is really about exposing one model (so it makes sense to wrap the object retrieval at least in part). We'll use the model identity for the default route component (not the constructor name).
  • I think it still makes sense to have application modules that contain multiple models, and have sub-modules that are
    • I think the modelDir('./models') will move into HasModels.
    • So a typical directory structure for an application might be (getting deeper, using sub-modules):
mymodule/
-index.js
-models/
  |-other.js
  |-thing.js
-templates/
  |-mymodule-whatever.ejs
-modules/
  |-other-view/
    |...
  |-thing-view/
    |-index.js
    |-templates/
      |-list.ejs
      |-view.ejs
      |-edit.ejs
  |-thing-admin.js
  • Routes for MVCModule: _list(req, res) is the root handler, calls _findAndPaginate(), passes that to list(req, res, objects) (but perhaps lets that resolve the promise, for further filter/populate?) and then template.render() with the returned context.

@loppear
Copy link
Contributor

loppear commented Aug 30, 2016

Offline discussion summary:

  • Yes, application modules should be able to have multiple models and their associated view classes proposed in this issue.
  • The boilerplate of loading ./models, ./templates will move to an ApplicationModule elsewhere.
  • MVCModule isn't really a nxus module (so don't put in sub-modules) and will be renamed Controller.
  • These should be auto-loaded by ApplicationModule for the same reason models are, so we'll make a standard directory ./controllers within an module to put these.
  • We'll have an analogous situation with AdminModel (AdminController?), likely they can all live in the same controllers folder.
  • Templates will stay in the parent ./templates directory and have naming conventions like mymodule-thing1-list.ejs, mymodule-admin-thing1-list.ejs (naming coming from the Controller implementation and if overridable it'll be there, same as routes).

@mjreich
Copy link
Contributor Author

mjreich commented Aug 30, 2016

👍 thanks for documenting

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

No branches or pull requests

2 participants