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

standardized controller/policies API #89

Closed
jaumard opened this issue Jan 17, 2016 · 26 comments
Closed

standardized controller/policies API #89

jaumard opened this issue Jan 17, 2016 · 26 comments

Comments

@jaumard
Copy link
Contributor

jaumard commented Jan 17, 2016

In order to make all Trailpacks compatible whatever the web server choice, we need a unified API for controllers and policies.

module.exports = class MyController extends Controller{
  /**
    * params : url parameters
    * query : get parameters
    * body : payload
  **/
  info(params, query, body, next){
    next(null, {status: 200, data: {whatever:'ok’}, headers:[]})
  } 
}

But actually controllers/policies are bind directly to web server so I don't see how we can implement this :/.
Any idea ?

@jaumard
Copy link
Contributor Author

jaumard commented Jan 21, 2016

How about run a function before web server was configure to check controller and if they extends a StandardizedController class, we generate a wrapper into each methods to make them compatible with the app.config.web.server

@tjwebb
Copy link
Member

tjwebb commented Jan 25, 2016

One idea might be to use the builtin Policy and Controller classes as the base types, and the webserver-specific types inherit from subclasses. e.g.

App using Hapi as Webserver

const HapiController = require('trailpack-hapi/lib/HapiController')
class ExampleController extends HapiController {
  exampleHandler (request, reply) {
  // ... hapi handler
  }
}

The "Trails Controller"

I think this is a good abstraction, but I want to emphasize in the documentation that it's only really necessary to use this pattern when building trailpacks. I don't want to signal to potential users that they might have to learn some new API to use Hapi or Express or whatever, when hapi and express already have perfectly good APIs that these potential users probably already know.

@jaumard I think since the trails router is based on hapi, I think it might make sense to model this Controller interface after hapi as well.

Now we have something like this:

Custom Trailpack that needs to work with all webservers, e.g. swagger:

const Controller = require('trails-controller')
class SwaggerController extends Controller {
  exampleHandler (request, response) {
    // implementation according to standard trails controller api
  }
}

Then it's the responsibility of the webserver trailpacks to implement the conversions. This would be easy for hapi, since the interface can be the same on both sides. Even though we'd have to build translation layers for express4, koa, et al, I think this is preferable to requiring all trailpack authors to build-in support for each webserver individually.

@jaumard
Copy link
Contributor Author

jaumard commented Jan 25, 2016

I'm ok with the fact the standardized controller should be learn/use only for trailpack. That's a good thing. In the doc we can only mention Trails controller on Trailpack part and never on Trails :)

Hapi(Express|Koa)Controller are not really needed... Controllers are map directly with web server so no need to have a specific class controller right ?

So if I understand you, under trailpack-express4 (for example) I need wrap all SwaggerController methods (cause it extends trails controller) and transform the express req, res to trails request, response ? I'm ok with this (smart to let web server trailpack do the wrapping) :) but we need to define request and response interfaces that can work with all web server.

@jaumard
Copy link
Contributor Author

jaumard commented Jan 25, 2016

Here my interface proposal :

request : {
  params: {name:value}, //URL params like {models}
  query:  {name:value}, //URL request like ?name=value
  body: {}, // Body of the request, post data for example
  files : {}, //uploaded files
  header: [], // request header
  route : '', //matched route
  hostname: '', hostname
  url : '', //full url
  isAjax : true/false // Request need json answer, or wantJSON...
}
response: {
  send: function(data){}, // Send data back
  render:function(viewName, data){}, // Send view
  redirect: function(to){} // Redirect to an URL
  status: function(status){}//Set response status
  header: function(status){}//Set response headers 
}

We can create 2 classes for them (in trailpack-webserver) and each web server trailpack extends them to be compatible.

@tjwebb
Copy link
Member

tjwebb commented Jan 26, 2016

I think it might make sense to try to re-use the hapi interface as our own generic interface, and conform other webserver trailpacks to it. This gives us three immediate benefits:

  1. the interface is already documented and the design is familiar
  2. there will be very minimal work to get trailpack-hapi working :)

There will be a slight performance hit on every request to go through this additional layer. If we use the hapi interface, then at least for the hapi trailpack, the performance decrease will be zero, since it can be a direct passthrough.

@jaumard
Copy link
Contributor Author

jaumard commented Jan 30, 2016

I don't like hapi interface but yeah ^^ it make sens to do it this way. I will start implement this in express4 and Controller template (https://github.com/trailsjs/generator-trails/blob/master/src/controller/templates/Controller.js) to remove the extends or all controller will be interpreted as Trails Standard Controller.
Agree ?

@tjwebb
Copy link
Member

tjwebb commented Jan 31, 2016

Sounds good. Honestly the hapi API isn't my favorite either (I don't know why body is called payload) but they put a lot of thought into it already and I'm happy to trust it.

@jaumard
Copy link
Contributor Author

jaumard commented Jan 31, 2016

Ok cool let's go on this ! :D we will be hapi to trust it ;) lol
I will need you for some spec with express4 when you have some time. About configuration and all middleware things.

@jaumard
Copy link
Contributor Author

jaumard commented Jan 31, 2016

@tjwebb ok first problem ^^ if controllers didn't extends trails-controller this.app is not bind into it.
So I think we need a trails-controller with a default constructor, a trails-standard-controller that extend trails-controller. If a controller is instance of standard controller class web server wrap interface like hapi. What do you think ?

@jaumard
Copy link
Contributor Author

jaumard commented Jan 31, 2016

Another thing, if I use this on trailpack-express4 :

const Controller = require('trails-controller')
if(controller instanceof Controller)//always false

I suppose trails-controller is a sub module of trailpack-express4 and controllers use trails-controller of the project so controller will never be instanceof Controller.

On my unit test all pass cause controllers are instance of trails-controller but on Trails projet nothing pass.

Did you have a way to fix this ? Or an idea ?

@jaumard
Copy link
Contributor Author

jaumard commented Jan 31, 2016

Ok for now I use this to detect if Controller is a trails-controller :

isTrailsStandard: function(obj) {
    let className = obj.constructor.name
    if (className == Controller.name || className == Policy.name) {
      return true
    }
    else if (className == 'Object') {
      return false
    }
    else {
      return this.isTrailsStandard(obj.__proto__)
    }
  }

I was able to run swagger Trailpack on express4 without problem :) that's a start ^^

@tjwebb
Copy link
Member

tjwebb commented Feb 13, 2016

Cool! I think this kind of method may be useful in other trailpacks as well. Would you mind adding this to lib/trails.js and adding a couple tests? I'm planning to expose a app.util object which will contain some methods like this.

@jaumard
Copy link
Contributor Author

jaumard commented Feb 17, 2016

@tjwebb Yeah sure in witch repo you want me to add this ? But in order to do it we need to create the trails-standard-controller (or whatever ^^) class

@jaumard
Copy link
Contributor Author

jaumard commented Apr 20, 2016

@tjwebb: are you ok if I create a repo trailpack-controller trailpack-policy (maybe trails-standard prefix is better ?) to by like trails-controller and trails-policy but for trailpacks ?

Related issue : trailsjs/trailpack-router#47 who block me to continue this.

@jaumard
Copy link
Contributor Author

jaumard commented Nov 18, 2016

So the solution for this is to create a dummy Request/Response object with same interface as hapi and under the hood each web server will implement his own API.

I don't know hapi interface enough to create those interface but if someone can help me create them I can implement them into express and we'll have a unified controllers/policies for hapi/express. #needHapiDev ^^

@jaumard
Copy link
Contributor Author

jaumard commented Nov 19, 2016

Blocked by trailsjs/trailpack-express#42

@tjwebb
Copy link
Member

tjwebb commented Jun 25, 2017

Hey all, thanks for contributing your thoughts. We've been able to think about this awhile, and I've tried to see how this would fit into the overall strategy of trails. After reading through these threads again, I think this kind of interface remains out of scope for the project.

It'd certainly be nice if our controllers/policies were re-usable, but this re-usability would not come for free, and I think the cost may not be worth the benefit. Re-usable logic can and should be implemented in Service classes. In designing our systems, it is sometimes desirable for some components to be disposable in order to maintain simplicity and focus, and I think that is the case here.

@tjwebb tjwebb closed this as completed Jun 25, 2017
@jaumard
Copy link
Contributor Author

jaumard commented Jun 28, 2017

@tjwebb I understand your decision about this common interface and you're right. But in that case we should be able to provide multiple version of controllers/policies under trailpacks to be compatible with multiple server. For example I can provide express and hapi controllers/policies under my trailpack and it will take the correct one to load depending on the one installed on the project, and both controllers/policies will use the common re-usable services.

For me it's an acceptable solution. But currently we don't have any way to make a trailpack compatible with both easily and it's a shame in my opinion.

Should I open a new issue about this ?

@clayrisser
Copy link

Maybe a trailpack could solve this problem. We could create a trailpack-bridge that would implement a standardized api between common node servers. Then, developers could choose to create a trailpack that ties into this trailpack-bridge api. In order to use trailpacks that use the trailpack-bridge api, you would have to install the trailpack-brigde, but it would be a compromise that would solve this problem while still following the Trailjs design philosophy, keeping Trailjs a simple, minimal, framework like it was designed to be.

@jaumard
Copy link
Contributor Author

jaumard commented Jun 29, 2017

@jamrizzi the chosen solution to have a trailpack-controller and trailpack-policy was the best approach having a trailpack that do this purpose is overkill as it's just two classes.
At some point I started with this trailsjs/trailpack-express#42 but I can be something that can be heavy to maintain (even under a trailpack) and maybe the team don't want it which make sense because controllers should only call services with params... That's why we need to be able to provide both express/hapi version of controllers and it should be able to take the correct one

@clayrisser
Copy link

Could you rephrase this response. I didn't fully understand it.

@jaumard
Copy link
Contributor Author

jaumard commented Jun 29, 2017

Sorry for my english :)
The Trails team decide to use hapi as default web server, so in order to be compatible with other web servers like express, the controllers/policies embedded into trailpacks need to respect the hapi request/response interfaces if you want one controller/policy.
But another solution is giving the possibility to trailpack to embedded two version of each controllers/polcies, one for hapi and another for express. Then the system will pick the correct one depending on which one is installed.

I started to try to create a wrapper hapi to express for trailpack-express but didn't manage to do it. Also it will be a bit painful to maintain when hapi or express change there API. That's why I think that embedded multiple versions is more the way to go here.

@clayrisser
Copy link

Thanks for the explanation. I think I'm understanding it. What do you mean by "embedded multiple versions". So, when creating a trailpack, if I build it using the hapi interface, it will work on express???

@jaumard
Copy link
Contributor Author

jaumard commented Jun 29, 2017

I mean under the trailpack you will have MyControllerExpress and MyControllerHapi.
The purpose of the express issue was that yes create trailpack controller/policies only with hapi interface and make them work on express trailpack. But having MyControllerExpress and MyControllerHapi will be an easiest solution and more easy to maintain also. Actually we have to way to really do that.

@clayrisser
Copy link

Is that currently implemented? If not, how can I help?

@jaumard
Copy link
Contributor Author

jaumard commented Jun 29, 2017

No nothing is implemented ^^ it will touch some core feature so the @trailsjs/maintainers team should be ok to do something for this.

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

3 participants