Make triggerMethod async

[jamesplease]

before:close should finish before the thing closes, whether it's async or not. How to do this?

This is pseudo-code that prob doesn't work, but something like:

Promise(this.triggerMethod('before:close')).then(keepGoing);

would be awesome. If triggerMethod returns a promise, then it waits for it to resolve before it keepGoings. Otherwise, it makes an immediately-resolved promise from whatever value is returned and executes it like normal.

This is one possible solution to the issue of animated regions, I think, @samccone.

[samccone]

https://github.com/bookshelf/trigger-then

[jamesplease]

Here's a mockup of my idea.

The idea is simple. Use a thenable chain in for our triggerMethod calls.

In the below example, X can be any class event, like a region's "show", or a "destroy" method. They would all look the same:

function X() {

  // In the Marionette world this would be triggerMethod('before:X');
  $.when(this.onBeforeX())

   // this._X would be where stuff actually happens. Either the showing, or the rendering, or w.e.
   .then(this._X)

   // Lastly, triggerMethod('x');
   .then(this.onX)
}

If you don't return a thenable from any of the callbacks, then the promise chain continues as if it were synchronous. In this way you retain backwards compatibility. However, if you do return a promise then the chain waits. So you can utilize onBefore methods to do more useful things, like handle animations in a region.

[yethee]

Async triggers can lead to decrease performance. Ref to Marionette.Async.

[jamesplease]

This is a really bad example (i h8 jsfiddle) to show easy it is to get animated regions with this.

[jamesplease]

Async triggers can lead to decrease performance.

heh, yeah. That's to be expected I guess. > 1 method calls will always be slower than 1 method call. I guess it's a matter of how much performance is lost and how much functionality is gained. I think Marionette's lack of support for animated transitions set us back when compared to, say, Angular, so I'd love to see a solution that fits in with the Backbone/Marionette paradigm. This is just one idea, but maybe the performance loss will be reason to abandon this approach. Definitely something to keep in mind.

[jasonLaster]

@jmeas - i think this would be an example where spec/performance is needed.

[yethee]

I had problem with closing of collection view when used Marionette.Async. A page was freezed for a along time when a collection view contains too many (> 100) children views. As far as I understood, the problem was in the close method, which is called async for each views.

And jQuery is not the fastest implementation of promise pattern. If Marionette will be support async triggerMethod, then need ability to change implementation of used promises and the sync function ($.when in jQuery context).

Sorry, for my bad english.

[jamiebuilds]

@yethee Performance is definitely going to be a concern, but I don't think it would be a huge problem if handled properly.

---

I would like to see a Marionette.Promise that you can set if you want to use another Promise lib

[jamesplease]

:+1: @thejameskyle

[yethee]

Async triggers can be useful in some cases but this feature should be optional, as I think. jsperf, comparison sync and async triggerMethod. Also will need sync cascade operation between parent view and nested views (when closing collection/composite view, for example), this adds cost of performance.

close: function() {
  var that = this;

  return Promise.all(this.closeChildren()).then(function() {
     that.triggerMethod("collection:closed");

     Marionette.View.prototype.close.apply(this, arguments);
  });
}

[jamesplease]

:+1: @yethee – thanks for making those great tests! Does anyone have any clever ideas on how to make this optional? The obvious ways I can think of are pretty gross...either duplicating all of the classes or adding if statements before every function that involves trigger methods.

[jamesplease]

Okay so here's an idea to make it optional.

First, let's start with the API. The user opts in or out with an instance-level option:

// Opt in to get async
var asyncRegion = new Marionette.Region({
  async: true
});

// Or go sync
var syncRegion = new Marionette.Region({
  async: false
});

// Sync is the default, so it isn't breaking behavior in this regard
var syncRegionTwo = new Marionette.Region();

To accomplish this, we introduce a new concept: Class Events (or something). We already have the concept of a class event, but we don't really say it in our API. A Class Event is just an important action that is wrapped in a before and after triggerMethod. At the moment, our Class Events are coded up like this:

// This is a mock of a region's show for illustrative purposes
show: function(view) {
  this.triggerMethod('before:show');
  this.$el.empty();
  this.$el.append(view.$el);
  this.triggerMethod('show');
}

The method we call does the stuff we need it to do and just begins and ends with triggerMethod.

This implementation I'm describing here would require rewriting all of these functions.

show: function() {
  this.classEvent('show', _show);
}
_show: function() {
  this.$el.empty();
  this.$el.append(view.$el);
}

What this does is separate out our methods so that we have greater control over them. The three things that need to happen are the before triggerMethod, the actual stuff, and then the after triggerMethod.

At this point you might be seeing where I'm going. The implementation of classEvent might look something like:

// Might be pseudocode; just typing it quickly
classEvent: function(name, method) {
  if (this.async) {
    this.triggerMethod('before:'+name).then(method).then(_.partial(this.triggerMethod, name));
  } else {
    this.triggerMethod('before:'+name);
    method();
    this.triggerMethod(name);
  }
}

Issues with this:

1. Majorly breaks bc as all of our public methods are completely rewritten
2. Sometimes we have deeply nested classEvents. For instance, a render Class Event might have different components to it, like rendering the template and rendering the collection.

Benefits to this:

1. Async and sync living happily ever after, and implemented in a way that's mostly hidden from the user
2. Separates out the part that does stuff from the triggerMethods, so if you want to overwrite, say, show, you can do that without having to worry about calling triggerMethod. This is a good thing, I think.
3. Negligible performance loss for sync triggerMethods

[rhubarbselleven]

IMO the foundation for this request is better tools around event queue management from within core.

The event queue model currently being used is FILO but as described here, we could want an Ordered queue, or even in odd cases FIFO to help control the execution order. Each part of core, eg a Region would inform something like wreqr what queue type it was after.

[jamiebuilds]

@jmeas I like the idea in theory. But I'm very against replacing/aliasing every method like that. I'd like to figure out something simpler.

[jasonLaster]

@jmeas I think this would add a tremendous amount of indirection in our code - this will come up when people are trying and find a method that's wrapped also event that's concatonated.

On Tuesday, June 3, 2014, James Kyle notifications@github.com wrote:

@jmeas https://github.com/jmeas I like the idea in theory. But I'm very against replacing/aliasing every method like that. I'd like to figure out something simpler.

— Reply to this email directly or view it on GitHub https://github.com/marionettejs/backbone.marionette/issues/1330#issuecomment-45040601 .

Jason Laster Software Engineer Etsy

[jamesplease]

Ha yeah, I'm not a big fan of it, either.

I think async trigger methods would be the most ideal solution to the problem of animated regions, but I don't think there's any way to implement them in a reasonable, performant way. With hope we can come up with a solution for animated regions that's as good...but, for now, I'm going to lay this issue to rest.

[samccone]

Reopeneing since I do not think this is resolved just yet.

[jamesplease]

I'm now :-1: to this idea.

Marionette has lots of triggerMethods, but in 99.9% of cases we only want the ones related to a region showing to be asynchronous because it helps with animated regions.

I closed this issue because of the difficulty in implementing async methods. To implement it you make great sacrifices in performance and code clarity while breaking BC in major ways.

Instead of implementing async triggermethods across the board we can introduce animated regions in an alternative way, as shown here. This implementation is:

• 30 more lines of code than current regions
• passes all unit tests
• backwards compat with current regions...simply swap in the new with the old
• highly customizable. use any animation library you want with it

If we adopt this solution for animated regions then I wouldn't see a need for async triggerMethods.

[rhubarbselleven]

So what we're talking about here is perhaps a User use case scenario to extend Regions to provide animation support.

[jamesplease]

@samccone I think this issue is dead per my comment above. It shouldn't be core...triggerMethods only make sense when they're synchronous. Are you fine with us closing this?

[jamiebuilds]

:+1: for closing