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

Add _triggerEventOnBehaviors to behaviors #2884

Open
Seebiscuit opened this issue Jan 19, 2016 · 6 comments
Open

Add _triggerEventOnBehaviors to behaviors #2884

Seebiscuit opened this issue Jan 19, 2016 · 6 comments
Milestone

Comments

@Seebiscuit
Copy link

Seebiscuit commented Jan 19, 2016

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.

@JSteunou
Copy link
Contributor

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.

@paulfalgout
Copy link
Member

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

@Seebiscuit
Copy link
Author

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

@paulfalgout paulfalgout added this to the v3.x milestone Jan 20, 2016
@Seebiscuit
Copy link
Author

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)

@paulfalgout
Copy link
Member

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.

@Seebiscuit
Copy link
Author

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

@paulfalgout paulfalgout modified the milestones: v3.x, v4.x Aug 26, 2017
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