backbone.marionette icon indicating copy to clipboard operation
backbone.marionette copied to clipboard

Add `_triggerEventOnBehaviors` to behaviors

Open Seebiscuit opened this issue 8 years ago • 6 comments

So, I find myself nesting behaviors (mostly to keep my code DRY). Sadly, while I can do this.view.triggerMethod on my parent behavior to reach a nested behavior method1 this method is brittle for two reasons:

  1. It triggers on all view behaviors, making it too general ~~2. More importantly this.view inside a behavior is not necessarily a Marionette.View, but a parent behavior, leading to unexpected...err...behavior~~

I think a quick and dirty fix would be to: a. Add a reference to nested behaviors in the parent behavior. It should be as "easy" as editing Marionette.Behaviors.parseBehaviors

    // Iterate over the behaviors object, for each behavior
    // instantiate it and get its grouped behaviors.
    parseBehaviors: function(view, behaviors) {
      return _.chain(behaviors).map(function(options, key) {
        var BehaviorClass = Behaviors.getBehaviorClass(options, key);

        var behavior = new BehaviorClass(options, view);
        var nestedBehaviors = Behaviors.parseBehaviors(view, _.result(behavior, 'behaviors'));

        behavior._behaviors = nestedBehaviors;

        return [behavior].concat(nestedBehaviors);
      }).flatten().value();
    },

b. Adding a triggerMethod method in Marionette.Behavior. Like this:

  triggerMethod: function() {
    var ret = Marionette._triggerMethod(this, arguments);

    this._triggerEventOnBehaviors(arguments);

    return ret;
  },

  _triggerEventOnBehaviors: function(args) {
    var triggerMethod = Marionette._triggerMethod;
    var behaviors = this._behaviors;
    // Use good ol' for as this is a very hot function
    for (var i = 0, length = behaviors && behaviors.length; i < length; i++) {
      triggerMethod(behaviors[i], args);
    }
  },

Look familiar? It should since I lifted them from Marionette.View almost verbatim2.

/cc @jasonLaster @samccone

1 _triggerEventOnBehaviors reaches all views, since it iterates over the container view's this._behaviors which holds references to al behaviors. 2 Removed this._triggerEventOnParentLayout(arguments[0], _.rest(arguments)); from Marionette.View.triggerMethod since it's not relevant.

Seebiscuit avatar Jan 19 '16 20:01 Seebiscuit

To second that, I'm not a big fan of accessing view from behavior. Maybe a little more abstraction on all the cases that today force us to do this.view... could add some benefits and also solve the cases of nested behaviors.

JSteunou avatar Jan 20 '16 07:01 JSteunou

I don't think that #2 is correct. Perhaps a failing test could prove this, but just looking through the parse behavior, the view that gets attached to the view's behavior is the same view that gets passed down to the nested behaviors.

That said, it seems reasonable that a behavior could trigger up to it's nested behaviors, just like a view can trigger up to it's behaviors. However, we should probably make this implementation off of the next branch for either v3.0 or v3.x

paulfalgout avatar Jan 20 '16 19:01 paulfalgout

Sorry @paulfalgout, you're absolutely right, #2 is incorrect. this.view inside the behavior belongs to the view that contains the top-most behavior.

Seebiscuit avatar Jan 20 '16 19:01 Seebiscuit

So...should I push a PR in next or is there protocol before I do that?

(This would be my first marionette PR, excuse my ignorance)

Seebiscuit avatar Jan 20 '16 19:01 Seebiscuit

Yeah branch off of next and PR into it :-)

You'll be happy to know that next is all node v4+ so all of the jsdom issues for testing should be resolved for windows on that branch.

You'll also notice the behavior code is slightly different now and utilizes a mixin on the view. You may also want to check out the triggerMethod implementation inside view, as I suspect it has changed slightly.

paulfalgout avatar Jan 20 '16 19:01 paulfalgout

@paulfalgout your memory is superb. And, yes, giddy. Thanks :smile:

Seebiscuit avatar Jan 20 '16 19:01 Seebiscuit