Do not bind class instance functions

[robertrossmann]

When loading controllers, services etc., the classes are instantiated, but then the instance will receive its own set of functions defined on the prototype which are bound to that particular instance. This is done here.

Is this really necessary?

I think I understand the intention here (avoid issues when those functions are called without context), but I would like to challenge the decision to support this kind of practice. To me, if someone tries to grab a method from a class instance, call it without its context (this) and expect it to just work, they are doing it wrong.

If my understanding is correct, the decision to use classes for controllers, models, services etc. has been quite recent so this might just be a remnant of some now-obsolete practice.

I would really love to get rid of this. :) Will you accept A PR that removes such functionality and instead just creates a new instances of all the things?

[scippio]

👍

[tjwebb]

To me, if someone tries to grab a method from a class instance, call it without its context (this) and expect it to just work, they are doing it wrong.

Good point, though this was done for a specific reason aside from saving users from themselves. Maybe you have some better ideas. More info follows.

In Hapi, for example, you don't get to send in the context and method separately when creating a handler. See: https://github.com/trailsjs/trailpack-router/blob/master/lib/util.js#L14. The handler is a method, so when it is invoked, this will be not defined. The explicit bind solves this.

As far as I know, express has a similar design: expressjs/express#782.

If my understanding is correct, the decision to use classes for controllers, models, services etc. has been quite recent so this might just be a remnant of some now-obsolete practice.

I ran into this issue even when using classes. If you can offer a solution, I'd certainly be willing to look at a PR with a more elegant solution.

[robertrossmann]

Interesting thing with how Hapi and Express deal with routes, did not know that.

Today I realised that using classes for controllers is not that great, because:

• As you point out, both of the aforementioned frameworks do not preserve context.
• Having the controller instance as this might not be ideal because it would be shared across all HTTP requests. If someone stores some properties relevant for the current request on this (the controller), it might not have the expected result (2 clients accessing same route suddenly share state).

Therefore I suggest a radical change: Stop using classes for controllers 😱 and instead advocate a plain object with simple functions (ie. what Sails currently does). Controller actions usually only serve as a kind of "dispatcher" and they do not require any state except that which is already provided by the req and res objects.

Optionally, the controller actions can be bound to the Trails' app, providing easy access to whatever might be required to handle the request. If this is not desirable, then having the app available on req should suffice (ie. req.app), leaving this simply undefined. The same should apply to policies and anything else that is ultimately treated as an http middleware.

Services and anything else should stay as classes, that makes sense for them.

[tjwebb]

Having the controller instance as this might not be ideal because it would be shared across all HTTP requests. If someone stores some properties relevant for the current request on this (the controller), it might not have the expected result (2 clients accessing same route suddenly share state).

Sure, but this would be an issue regardless of how we bind context, or whether or not we use class. Because I've decided I don't want Trails to be a global variable, the app must be accessible using this. So even in a "pure" function, you could still say this.foo = 'bar'.

We could use a similar strategy as the one I'm using to lock down the config object, by using Object.freeze. This way the app would immediately fail if the programmer tries to modify or add properties to a Controller's this context.

[robertrossmann]

The thing with controllers is that the controller's instance (this) has no real value in route handlers - you cannot put there anything related to current request (that should go into req and res input params), and everything else you might need to handle the request is usually available either via this.app (the Trails instance) or by explicit requires/imports.

Therefore, the route handlers' execution context can be bound directly to app (alternatively, the app could be made available on req.app, like in Sails, but I do not really like that approach).

And now, if we decide to bind the context directly to app, I find it strange to use classes for defining route handlers - it now seems more appropriate to define them as plain objects with functions. 😄

Also, some people might think, seeing that controllers are defined as classes, that each request will receive its own instance of that controller, which is not true.