Change semantics of `Controller#beforeAction`

[paulmillr]

Current:

class Controller
  beforeAction:
    '.*': (params, route) -> 
      @compose 'site', SiteView

    'show': (params, route) ->
      @compose 'navigation',
        compose: ->
          @model = new Navigation
          @view = new NavigationView {@model}
        check: ->
          console.log arguments
          yes

Proposed:

class Controller
  beforeEach: (params, route) ->
    @compose 'site', SiteView

    if route.action is 'show'
      @compose 'navigation',
        compose: ->
          @model = new Navigation
          @view = new NavigationView {@model}
        check: ->
          console.log arguments
          yes

Basically current form is no longer needed since we pass route information. Switching to classical method will:

• Decrease complexity and ambiguity
• Increase performance (walking the whole protochain is costly)

[molily]

I never liked the ugly '.*': … part – it reminds me of Japanese emoticons, btw '.+' would be a saner RegExp –, but apart from that I like the declarative style. The alternative is a bunch of if else statements or a big switch statement:

if route.action is 'show'
  …
else if route.action is 'index'
  …
else if route.action is 'new'
  …

That’s not so nice to write and read either. So I don’t see a big improvement in having one big method. But I would be fine with both versions. For most before action code, the difference won’t be great.

The implementation in Chaplin would get simpler, that’s right.

[paulmillr]

@molily the thing is — route actions IMO are not as useful as, for example, route names or controllers. It can be tough to filter through them.

[mehcode]

In all of my apps so far all I ever use is beforeAction: '.*': -> .... I don't make use of the filtering anyway and removing would speed things up. I don't see the action filtering to be used very often and as its not common I agree with removing it.

[andriijas]

I had one case where I didnt use .* but that code ended up in a derived controller using .* anyway.

If the performence boost is massive, Im all for it. Otherwise Im a fan of keeping methods small ie not if foo else if bar else qux

Its a shame coffeescript doesnt have decorators like python :)

[molily]

We have be careful about going back and forth with features. Chaining before actions asynchronously with Promises was introduced some months ago (0.6.0 I think). I’d like to get some user feedback on this before we throw it out again.

Chaining Promises can be done with one beforeAction method as well (jQuery 1.8+ Deferreds should suffice for simple chained async I/O):

beforeAction: ->
  @authenticateUser().then(@checkAccessRights).fail(@redirectNotAuthorized)

[paulmillr]

@molily my patch preserves promises compatibility, with single beforeaction

[andriijas]

1) Why drop support for prototype chain traversal when a) Chaplin already sits on quality code for this b) This is one of Chaplins no 1 marketing value: http://chaplinjs.org/ second bullet better support for inheritance

2) What is the purpose of initialize on controllers if we are going to have one beforeAction method? Before beforeActions I used initialize as the poor mans beforeAction but to make it useful it needs to be adjusted to behave like a before action, Ive seen no real world use case for the current controller initialize.

3) I think promises are really useful. In our admin portal/intranet/whatever I find it useful to fetch a collection of pricelists/users for a customer/other things and reuse the model from the collection in the item view (controller#show) instead of doing an additional fetch. Users of this portal (my collegues) often works with "models one by one" so to have the whole collection preloaded makes sense.

4) If we are going the beforeAction: -> way how do we deal with promises and super call?

5) What is it that we really want to achieve by changing the semantics? Better performence? Easier code maintenance (all tests are already there though). Better readability?

6) I like the strategy that things get shipped and then revisited a couple of releases later when it has been used in real world apps for a while

7) Action specific stuff could be handled manually like:

show: (params, route) ->
  beforeShow(params, route).done ->

[paulmillr]

1 — it is reasonable for events, but it should not be used everywhere. 2 — it’s not needed, we can leave it unused 3 & 4 — my patch preserves promises compatibility. super is obviously supported. 5 — perf, readability, simplicity. I despise complexity in architectures. Current implementation has unnecessary complexity. Implementor (Ryan) said he wanted to have simple function back then too. 6 — we will stop breaking compat after 1.0 which should be after 0.9. Actually I created this issue because I started using before actions in real code and they were painful

[andriijas]

3-4) just so I get it right, this is how it would work then?

bazooka = super
dfd = new $.Deferred()
bazooka.done -> @explode() and dfd.resolve()
dfd

Might not be very often one would like to do something like that though.

[paulmillr]

yea, something like that

[mehcode]

Before actions serve a few purposes. Executing arbitrary code (such as compose) before action methods. Authenticating the user asynchronously (eg. using SAML) with deferreds to halt execution until a resolve or reject. Both of these cases are still supported and recommended.

The current implementation of beforeAction additionally allows for a different before action based on route action. What @paulmillr is saying with this patch is that he has had no need for this additional capability. I have not either. Furthermore this capability comes at a severe performance cost because of the mandated prototype walking and regex matching for each request. Additionally our dispatcher code is a mess right now and will be cleaned significantly.

If we had already shipped version 1.0 we would deprecate beforeAction with loud deprecation messages for at least one additional minor release and remove it on the next major release to follow semver. As we have not shipped version 1.0 we can remove it immediately.

---

@andriijas

beforeEach: ->
  deferred = new $.Deferred
  super.done (data) -> @explode(data).done -> deferred.resolve()
  deferred

About as flexible as:

beforeAction:
  '.*': (params, route, options, data) ->
    deferred = new $.Deferred
    @explode(data).done -> deferred.resolve
    deferred

And with less magic.

[andriijas]

No no by all means, improve as much as you like, even after 1.0. I like living on the bleeding edge. Just wanted to get a feeling if and how this changes my app code :) And Im actually serious about getting stuff released and then revisit after a while. Maybe the same approach should be done with #441 since 1.0 isnt here yet.

Thanks for the examples @mehcode, beautiful! Though now I have to type check if the return value of super is a deferred or something else. I mean if a user is already authenticated you probably dont return a deferred, or?

Thats what I think is super sweet today, derived actions doesnt need to care about parental beforeActions. Its a minor though, given the complexity we get rid of within the dispatcher. We lose some fat here, some is added else where.

[davidkaneda]

I don't stay on the bleeding edge of development, but just to throw in two (probably off-topic) cents here...

I'd love to see deferreds be able to be used in conjunction with dispose as well — creating "stack"-like animations (where one view animates into another view) is a constant need, and the way Chaplin currently disposes views when controllers switch makes this a bit of a pain.

Also: I'd be curious how much weight it would add to polyfill $.Deferred for Zepto users...

Just some food for thought, keep up the great work! :)

[andriijas]

@davidkaneda view transition is handled in #105

[davidkaneda]

Awesome, thanks for the reference, will check it out soon (I'm still on .6, ha, so I know there's plenty new things to explore)

[mehcode]

Though now I have to type check if the return value of super is a deferred or something else. I mean if a user is already authenticated you probably dont return a deferred, or? [...]

That's a good point. Here is how I would adapt my current code.

# top-level controller
protected: true
beforeEach: ->
  dfd = new $.Deferred()
  @publishEvent '!session:acquire', (session) =>
    # If the user is authenticated; this invokes the callback immediately.
    # session is a session object that is also attached to `mediator.session`.
    dfd[if not session.authenticated and @protected then 'reject' else 'resolve']()
    dfd = false
  dfd

# current controller
beforeEach: ->
  callback = => 
    # ...
  dfd = super
  if dfd then dfd.done(callback) else callback()

And here is what it would be if we kept the prototype walking.

# top-level controller
protected: true
beforeEach: ->
  dfd = new $.Deferred()
  @publishEvent '!session:acquire', (session) =>
    # If the user is authenticated; this invokes the callback immediately.
    # session is a session object that is also attached to `mediator.session`.
    dfd[if not session.authenticated and @protected then 'reject' else 'resolve']()
  dfd

# current controller
beforeEach: ->
  # ...

I don't know. @paulmillr The above case is convincing me that we should keep the prototype walking. I'd still like to remove the regex matching though.

[akre54]

Also: I'd be curious how much weight it would add to polyfill $.Deferred for Zepto users...

@davidkaneda not a whole lot :) https://github.com/Mumakil/Standalone-Deferred

[paulmillr]

@mehcode we can’t leave both. Someone will call super in method and things will break.

As for “checking if value is deferred” — unfortunately yes, but this can be solved by a super-simple utils helper in user application.

it’s sad that jquery implementation is the most popular, because it’s the most shitty. There are a lot of cool promise/A+-compliant libraries that can handle this stuff automatically.

[andriijas]

Theres that word again, shitty :-)

Im a super cool jQuery 2.0 beta user hero and I know it!

[mehcode]

it’s sad that jquery implementation is the most popular, because it’s the most shitty. There are a lot of cool promise/A+-compliant libraries that can handle this stuff automatically.

I'd love to see some good examples of some. We should be promoting the best in our examples. If its nice and simple then that obviates my concerns for removing the prototype walking.

[paulmillr]

@mehcode I’ve used https://github.com/cujojs/when and http://documentup.com/kriskowal/q/. When.js seems cleaner and nicer, but both are good.

[mehcode]

@paulmillr Looks nice.

beforeEach: ->
  deferred = when.defer()
  when(super).then (data) -> @explode(data).then -> deferred.resolve()
  return deferred.promise

The above looks like it should work nicely.

[paulmillr]

note that when is a reserved word in coffee

[chrisabrams]

js2coffee.org wants to change when() to when_() which I'm guessing would break the lib unless the author has a when_ = when setup.

[mehcode]

Meh. Good point.

_when or "when" doesn't look as nice. I'd stick with Q then as Q.when and Q.defer look nice and consistent (and still works in coffee-script).

beforeEach: ->
  deferred = Q.defer()
  Q.when(super).then (data) -> @explode(data).then -> deferred.resolve()
  return deferred.promise

[paulmillr]

i’ve used when_ back then

[molily]

No need to create another deferred and resolve/reject it manually. Chaining creates new Promises. Rejecting the current Promise works using exceptions.

class PrivateController extends Chaplin.Controller
  beforeEach: ->
    @authenticate() # Returns a promise
class AdminController extends PrivateController
  beforeEach: ->
    # Should be the same with Q.when
    whenjs(super).then(@checkAdmin).then(null, @notAllowed) # or `fail` in Q, `otherwise`in when.js
  checkAdmin: (user) ->
    throw new Error('Not an admin. Access denied.') unless user.isAdmin
  notAllowed: ->
    @redirectTo '/errors/401'

[andriijas]

Even jQuery should work then. Ive done a shit load of work at work cleanin up jquery plugins and structure up the client side app with chaplin. Perhaps after the summer Im ready to custom build jquery (or even drop it) and replace it with these minimal puppies. Right now I dont want to add more KBs just to get "nice promises"

class PrivateController extends Chaplin.Controller
  beforeEach: ->
    @authenticate() # Returns a promise
class AdminController extends PrivateController
  beforeEach: ->
    $.when(super).then(@checkAdmin).fail(@notAllowed)
  checkAdmin: (user) ->
    throw new Error('Not an admin. Access denied.') unless user.isAdmin
  notAllowed: ->
    @redirectTo '/errors/401'

Seems like we have reached consensus, lets get it merged!

[andriijas]

Quick thoughts after migrating app today:

1) It certainly easier to get the concept of beforeActions for an untrained eye, showed the difference to a backend colleague.

2) Its convenient to be using standard object oriented pattern with super

3) there is one big caveat to how it worked before which we should mention in docs, because of 2) beforeActions are not exectued top-down but down-top. If you previously did something like options.foo = bar in a "parental" beforeAction its not available in the current class beforeAction. And you have to more or less do the $.when(super).done in the whole chain you deal with promises for auth etc in the base controller.

From one point of view it makes more sense to execute beforeActions top-down as before, from the object oriented, easier implemented point it makes sense with the new implementation.

I dont know if my app is way out of standard use case but I already have up to 3 levels of beforeAction in my real world app. :)

[paulmillr]

what is “top-down”?

class A extends Controller then beforeAction: -> @compose 'shit', Shit
class B extends A then beforeAction: -> super; @compose 'shit2', Shit
class C extends B then beforeAction: -> super; @compose 'shit3', Shit

isn’t it top-down?

you can also do the reverse

class A extends Controller then beforeAction: -> @compose 'shit', Shit
class B extends A then beforeAction: -> @compose 'shit2', Shit; super
class C extends B then beforeAction: -> @compose 'shit3', Shit; super

[andriijas]

Japp something like that.

And if A returns promise you need to wrap super $.when() in both B and C if your both B and C depends on A

Its not a big problem but something that ppl who migrate need to be aware of.

Maybe I should just try to make my A not returning a promise, I think that would ease some of my headache.

[andriijas]

Just discovered https://github.com/chaplinjs/facebook-example/blob/master/coffee/lib/utils.coffee again, which probably cant be mentioned enough when dealing with deferreds. Superior utils.