trailsjs / trailpack-hapi

:package: Hapi.js Trailpack
MIT License
19 stars 9 forks source link

move responsibility of the "require" to the user #3

Closed tjwebb closed 8 years ago

tjwebb commented 8 years ago

see: https://github.com/trailsjs/trailpack-hapi/blob/master/lib/server.js#L28

A design distinction (which I've not made clear at all thus far) of Trails is that the user is responsible for requiring the modules they need. This is exemplified here: https://github.com/trailsjs/trails/blob/master/archetype/config/trailpack.js#L14-L19.

Even application modules like controllers and such are manually required: https://github.com/trailsjs/trails/blob/master/archetype/api/index.js. The generator is responsible for adding these require statements when new Controller, Model, etc is created (via yo trails:model or something). See: https://github.com/trailsjs/generator-trails/blob/master/src/app/util.js#L61-L79. I use falafel to parse the file into an AST, attach the new statement to the tree (if it doesn't exist), and then re-serialize the AST back into a Javascript string.

In Sails, there's a ton of module-loading magic that is completely unnecessary which I'd like to remove. I see the arguments for its superfluity as follows:

  1. Adds dependency bloat to the framework, since the framework and its modules installing these modules always, regardless of whether you plan to use them.
  2. Requires try/catch logic everywhere to attempt to the require the module, and then fail gracefully if it doesn't exist. This increases boot time of the application.
  3. Makes it difficult to statically analyze stuff, e.g. if we want to run this through a transpiler of some sort, it won't know what to require.
  4. At a philosophical level, it increases the opacity of the framework, while not offering any productivity or usability gains.
tjwebb commented 8 years ago

So in this case, I think an example views config would look like:

// config/views.js
{
  engine: require('whatever'),
}

The generator could set a default one, or ask the user during generation. But also, if engine isn't set at all, then none of this registerView stuff happens at all. Then if the user builds a View and then tries to access it, we return some sort of 500 error telling them they haven't installed a view engine.

tjwebb commented 8 years ago

Maybe we should move the views config into config/web.js actually?

weyj4 commented 8 years ago

Definitely agree about moving the requires into config. Not sure about config/web.js instead of config.views.js. What are the respective responsibilities there?

tjwebb commented 8 years ago

The view rendering engine of choice seems configurationally to be a property of the web server, and not really of anything else. It is web server-specific. I'm thinking something like

// config/web.js
module.exports = {
  // other server stuff
  views: {
    engine: require('foobar'),
    // more view stuff
  }
}

It's not too important, I just separated them by default because that's what Sails does. But looking at it now, I think it might make more sense to put them in one file.

tjwebb commented 8 years ago

We can tackle the web.js/views.js thing in a separate issue. Currently, this issue breaks a default installation until I manually install and set some view engine.

tjwebb commented 8 years ago

Hey @weyj4 are you available to look at solving this tomorrow? We can pair on it.

weyj4 commented 8 years ago

Hey, I just moved that require out. Let me know later on (after Star Wars) if you want to do more here. There's 2 things I'd like to get done: 1) decide on a default view engine and rewrite index.html and eventually the 404, etc pages inside archetype/views; 2) Get this sorted out and start serving a dist/ directory.

Also we can talk about reorganizing config/