Adds support for string-based itemEvents #875

jamesplease · 2014-01-21T00:50:48Z

There’s a new Marionette.helper in town, and it allows you to specify
function names as strings in the itemEvents hash as well as the usual
function references.

Fixes #872

jamesplease · 2014-01-21T00:56:38Z

I put the helper function that allows this directly on Marionette as it seems like something that could be used elsewhere. If I remember correctly it is only used in collection views in the Marionette core, so maybe it'd be better if it went there.

cobbweb · 2014-01-21T01:03:25Z

spec/javascripts/collectionView.spec.js

+      cv.render();
+    });
+
+    it("should not break", function() {


Y U So empty?

I couldn't really think of anything to test, you know? No function exists to test (which is the point), and nothing happens (which is also the point); I just wanted to verify that the code runs and doesn't 'splode. One might think to test that the eventsHash doesn't try to attach the function that doesn't exist, but it seems like that's all handled internally, so there's no way to extract it.

This seems like a significant-enough test for what I was trying to do. If you can think of another way to show this that you think is more substantial, I'd be happy to implement it.

👍 Yea I was confident that was the case, just good to get clarification. Might be worth commenting something along the lines of 'Intentionally left blank' in the test?

That's a good idea, @cobbweb. I added the comment.

jamesplease · 2014-01-21T01:32:37Z

Added the comment for that 'empty' test, tidied up some spacing, and removed unused variables in the tests. If either of you see anything else lemme know.

jamesplease · 2014-01-21T02:52:43Z

Fixed it up according to @samccone's suggestions. The function is now normalizeMethods, and the returned value is now normalizedHash

samccone · 2014-01-21T03:50:17Z

Update Readme spec 👍

jamesplease · 2014-01-21T03:57:25Z

Done!

cobbweb · 2014-01-21T03:59:20Z

src/marionette.helpers.js

+
+// Transform a mapping of events => functions or function names
+// and return a mapping of events => functions
+Marionette.normalizeMethods = function(hash) {


I think we should have some tests for this function directly too? ping @samccone

yeah unit test time (:

Can you add some @jmeas?

Yup. I'll have time tomorrow to get to it, I think. Sorry, should have said something 😄

cobbweb · 2014-01-22T04:43:40Z

NPM issues in the build, re-ran = green :)

jamesplease · 2014-01-22T14:28:07Z

Patience, friend! It'll be squashed once it's ready. I haven't added those tests yet.

samccone · 2014-01-22T14:30:00Z

heh no worries 👍 🅰️

samccone · 2014-01-22T14:30:22Z

@cobbweb you should be able to manually trigger a rebuild on travis

jamesplease · 2014-01-22T15:32:54Z

So I'm writing these tests, and (I should have seen this sooner), but the context of the normalizeMethods function isn't right. It's being called with this as Marionette, which breaks the function, since it relies on this being the object that's calling the fn. It's a bit tedious to always _.bind the function when you want to call it so something should change, I think.

One option is to attach it directly on Marionette.View, which makes it accessible to all of those things that extend from it (collectionViews being one of them). But since the function is just for general-purpose event handling, I can see people wanting to use it for Controllers or Modules (as components).

Another option (maybe better) would be to pass the context as the second argument. Marionette.normalizeMethods(hash, this);

I'm leaning more toward the 2nd of these options, but I'd like to know what y'all think.

There’s a new Marionette.helper in town, and it allows you to specify function names as strings in the itemEvents hash as well as the usual function references. s#Updated docs

jamesplease · 2014-01-22T18:58:35Z

All patched up.

Added more docs
Added a context parameter
Added more unit tests
Fixed the broken unit test

✓

lukesargeant · 2014-01-22T21:22:32Z

Is it worth mentioning pull request #790 whilst there is interest in item event bubbling stuff?

samccone · 2014-01-22T21:26:12Z

@lukesargeant this is not introducing new bubbling, rather a declarative/extendable method to react to events that are already moving up the view chain.

lukesargeant · 2014-01-22T21:41:13Z

Ok, I thought it might be worth a mention as item event bubbling is the more general topic but you're right it's not strictly related to the functionality this offers.

cobbweb · 2014-01-23T00:26:12Z

@cobbweb you should be able to manually trigger a rebuild on travis

Yea that's what I did :)

but the context of the normalizeMethods function isn't right.

This is the same pattern that triggerMethod uses. Each class adds it to their prototype like this.

jamesplease · 2014-01-23T00:27:45Z

Makes sense. I'll go ahead and do that.

jamesplease · 2014-01-23T01:53:25Z

Okay, @cobbweb, I updated the source. normalizeMethods no longer accepts a 2nd argument, but instead is added to the prototype of Application, View, Module, and Controller.

Anachron · 2014-01-23T01:59:36Z

@jmeas great, looks much better than before.

cobbweb · 2014-01-23T03:48:33Z

Sorry for the confusion, I don't think we want the normaliseMethods method attached to the prototype unless that prototype uses it. ping @samccone

jamesplease · 2014-01-24T01:04:44Z

Hmm, you're right, @samccone, that in most cases that code above would work, but I'm wondering if relying on the getOption method is the best idea here.

One concern I have is whether or not that method will be changed when this.options support is removed (per #769)? It'd be strange, I think, for Marionette to be sifting through a property that the user only optionally attaches. At that point, the user is really free to name options whatever they want, as that name will no longer even have a special meaning within the framework (outside of getOption), right? But maybe this situation is not actually as weird as I'm thinking it to be.

Another concern of mine is that relying on getOption here adds an extra level of complexity onto the normalizeMethods that doesn't seem necessary. Conceivably one could have an options.someFn that differs from this.someFn. Were I a person in that situation, I'd definitely expect normalizeMethods to grab the this.someFn, and not the options.someFn. Sure, this could be documented, but I still don't think the behavior of normalizeMethods there is ideal.

If code reduction is a concern one idea would be to separate out the value-searching functionality into a method shared between both getOption and normalizeMethods, though I haven't completely thought through how that might look.

jamesplease · 2014-01-27T15:59:00Z

To summarize what's up with this issue right now:

There's currently inconsistency between the structure of the internal methods of Marionette. If consistency is a desirable quality then some of them need to be changed to behave differently. This PR is hung up because there has no been a decision made on which implementation we ought to go with.

One option to pass the context/target object of the helper, like hasOption. In this case, you don't need to attach the method to any prototypes, as you call it directly from the Marionette object. The other option is to not pass the context and instead attach it to prototypes. This behavior can be seen with the triggerMethod function.

Here are two reasons I think converting things to behave like hasOption makes sense:

The first is that because they're internal most users shouldn't have to overwrite them or call them directly. If that's true, then it's also true that it's less lines to just pass the context instead of directly attaching it to the prototypes. Further, as the number of internal methods increases the number of this.someMethod = Marionette.someMethod lines increases on all of the prototypes, which seems like something I'd want to avoid. The counter-argument to this is that there really aren't that many internal methods, and there probably won't ever be.

Another reason to go with passing the context is that you don't need to decide which methods are attached to each prototype. This normalizeEvents function is pretty useful, I think, to anyone who wants to work with events beyond the basics that Marionette comes with. However, it only makes sense to attach it to the View prototype right now because only Collection Views explicitly use them. The counter-argument to this is that there's not really any reason to design the framework with alterations to it in mind. If the framework is intended to work in a given fashion, all you need to do is worry about it working like that.

Neither of those are incredibly strong reasons that I think passing the context really ought to be the way we go, but they are reasons to sway my opinion in that direction, which is more than I can say for the other option. But I'm sure there are a number of benefits to the other way, as well.

samccone · 2014-01-27T16:27:49Z

@jmeas you are really spot on.

The best way is to set the context on call, not to pass it in as I previously said.
triggerMethods is a perfect example of what we should be shooting for.

I agree with you that the abstraction is valuable and useful.

jamesplease · 2014-01-27T16:51:09Z

So, wait, maybe I'm confused, but I was suggesting the hasOptions has a few benefits over the triggerMethods implementation. In other words, having signatures along the lines of Marionette.someFn = function(context, hash); instead of having lots of this.someFn = Marionette.someFn lines.

Do you think otherwise, or did you just make a slip up in typing? :)

samccone · 2014-01-27T18:20:47Z

Ah I do think otherwise.

By using this pattern this.someFn = Marionette.someFn we are building our classes by composition, allowing for decoupled utility methods.

Passing the context in via an argument is also a valid way to do this, it is again just inconsistent.

As to your argument about polluting the prototype, we are really not polluting it per say, rather just storing a reference to the util method with the correct context maintained (but true we are adding a key onto it).

So your points are valid, however instead of breaking from the way we are doing it in this project let's just stick with the pattern in place.

jamesplease · 2014-01-27T21:58:26Z

I wasn't really worried about pollution, but rather just than just having lots of this.x = Marionette.x lines. But I think either way's fine, really.

@cobbweb, thoughts?

bobbyrenwick · 2014-01-28T17:13:28Z

👍 excited for this!

cobbweb · 2014-01-28T21:56:40Z

I'm fine with either way, but prefer calling the function in the right context versus passing an argument.

jamesplease · 2014-01-28T22:03:38Z

Alright, cool. It sounds like you and @samccone agree, and I don't feel too strongly either way, and that's what this PR is currently doing. If there's anything else you want me to change, let me know, but it looks good to me. 👍

samccone · 2014-01-28T22:12:06Z

src/marionette.module.js

@@ -18,6 +18,7 @@ Marionette.Module = function(moduleName, app, options){
  this.startWithParent = true;

  this.triggerMethod = Marionette.triggerMethod;
+  this.normalizeMethods = Marionette.normalizeMethods;


no need for this to be here right now right?

jamesplease · 2014-01-29T00:13:56Z

Ah, right, @samccone. I've removed the new function from every prototype except for view.

funkatron82 · 2014-01-29T06:01:47Z

Will this work for namespaced events like do:something?

jamesplease · 2014-01-29T12:28:05Z

Yessir

cobbweb · 2014-01-29T23:21:34Z

docs/marionette.functions.md

+      eventOne: "someFn", // This will become a reference to `this.someFn`
+      eventTwo: this.someOtherFn
+    };
+    this.normalizedHash = Marionette.normalizeMethods(hash);


Should this line be either this.normalizeMethods(hash)? Can you add that normalizeMethods is already added to Marionette.View but has to be added by the user if they want to use it in non-view classes?

good catch, it does need to be this.normalizeMethods(hash)

jamesplease · 2014-01-29T23:38:51Z

Good catch, @cobbweb. Updated! 👍

jamesplease · 2014-02-03T21:01:09Z

YOU EVER GONNA MERGE THIS, @samccone?

samccone · 2014-02-03T21:01:56Z

ha 💃 it is on

https://github.com/marionettejs/backbone.marionette/tree/dev

going to run it through a few apps

samccone · 2014-02-07T19:43:12Z

merged in dev and into master

cobbweb reviewed Jan 21, 2014
View reviewed changes

Adds support for string-based itemEvents

65e5ad4

There’s a new Marionette.helper in town, and it allows you to specify function names as strings in the itemEvents hash as well as the usual function references. s#Updated docs

samccone reviewed Jan 28, 2014
View reviewed changes

cobbweb reviewed Jan 29, 2014
View reviewed changes

okitu mentioned this pull request Feb 6, 2014

View.triggers should include the DOM-Event #903

Closed

samccone closed this Feb 7, 2014

jamesplease mentioned this pull request Feb 20, 2014

Update Marionette functions #927

Closed

Adds support for string-based itemEvents #875

Adds support for string-based itemEvents #875

Conversation

jamesplease commented Jan 21, 2014

jamesplease commented Jan 21, 2014

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jamesplease commented Jan 21, 2014

jamesplease commented Jan 21, 2014

samccone commented Jan 21, 2014

jamesplease commented Jan 21, 2014

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cobbweb commented Jan 22, 2014

jamesplease commented Jan 22, 2014

samccone commented Jan 22, 2014

samccone commented Jan 22, 2014

jamesplease commented Jan 22, 2014

jamesplease commented Jan 22, 2014

lukesargeant commented Jan 22, 2014

samccone commented Jan 22, 2014

lukesargeant commented Jan 22, 2014

cobbweb commented Jan 23, 2014

jamesplease commented Jan 23, 2014

jamesplease commented Jan 23, 2014

Anachron commented Jan 23, 2014

cobbweb commented Jan 23, 2014

jamesplease commented Jan 24, 2014

jamesplease commented Jan 27, 2014

samccone commented Jan 27, 2014

jamesplease commented Jan 27, 2014

samccone commented Jan 27, 2014

jamesplease commented Jan 27, 2014

bobbyrenwick commented Jan 28, 2014

cobbweb commented Jan 28, 2014

jamesplease commented Jan 28, 2014

Choose a reason for hiding this comment

jamesplease commented Jan 29, 2014

funkatron82 commented Jan 29, 2014

jamesplease commented Jan 29, 2014

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jamesplease commented Jan 29, 2014

jamesplease commented Feb 3, 2014

samccone commented Feb 3, 2014

samccone commented Feb 7, 2014