wrong return-value when mixing in a previously-unknown method name

[rjharmon]

  myMixin = function() {
    return { foo: function() { return "My foo result" }
  }
  myOtherClass = function() {}.mixin(myMixin); // mumble - point is, it doesn't have 'foo'.
  myOtherClass.foo("some arg");  // returns "some arg"

I expected this to return "My foo result".

Currently, I have to specify 'clobber' for a function name that didn't exist before, because the identity function's return value is returned instead of my function's return value. Surprise! and :(

I'd sort of hope that after would recognize the no-original-method case, and take a shortcut whereby we just create the method with direct reference to the new method. Methods aside, the outcome still seems wrong.

[rhysbrettbowen]

I can see how that'd come as a surprise - though in this case I think it might actually be a good thing. The more I wrote mixins the less I used clobbering functions. A function that clobbers goes against the whole idea of composing. If somethings a clobber it should really have been put on the prototype. The issue is that an after doesn't return it's value so if this was changed then the mixin would behave differently depending on the object you put it on, which is not what we want. It's better to have that "clobber" in there and know what is going to happen (although I would really recommend putting it on the prototype or making it an around so you can return the new value and keep whatever was there, something like:

around: {foo: function(fn) {fn(); return "My foo result";} }

[rjharmon]

I don't want to use clobber; I want to be able to specify a function name in the mixin, that's not expected to be found in the target class, and have its return statement work.

If that function does already exist, I'd say it should throw an error at the developer, and allow them to provide mixin options indicating how they want the conflicting function to be resolved, e.g. { clobber: "conflictingFunctionName" } or { before: "conflictingFunctionName" }.

WDYT?

[rhysbrettbowen]

"after" is the default but it's not meant to alter the return value, just run something afterwards. In the case where you want to have control of a return value then you should always use "around". If you're using around then you have control of the return type and the function that gets passed in so you won't have the issue.

I think the problem here is that it may be tricky to realise when to use around/after and which is the default. Do you think it would help if there was something about when to use each one and best practices in the README?

[rjharmon]

I understand if you have an attachment to the current default behavior or
even if it's politically difficult to contemplate a change, considering
other client code relying on Backbone.Advice.

That entirely aside, I rather think the behavior default is surprising and
wrong, and not a good thing by any stretch of the imagination. By
specifying a name at the top level of the mixin (no options), the author's
intent is, I would wager over 90% of the time, "I don't expect this
function to be a problem" and "I expect my return statement to work".
Having to add "around" to get a return statement to work is a smell.

If you can't actually change the behavior due to other factors, I
understand. I believe that objective evaluation leads to the perspective
I've shared. I love clean code and appreciate the separation of concerns
and code-sharing enabled by Backbone.Advice, and I thank you for sharing it
regardless.

On Wed, Jun 11, 2014 at 1:16 PM, Rhys Brett-Bowen notifications@github.com
wrote:

"after" is the default but it's not meant to alter the return value, just
run something afterwards. In the case where you want to have control of a
return value then you should always use "around". If you're using around
then you have control of the return type and the function that gets passed
in so you won't have the issue.

I think the problem here is that it may be tricky to realise when to use
around/after and which is the default. Do you think it would help if there
was something about when to use each one and best practices in the README?

—
Reply to this email directly or view it on GitHub
#14 (comment)
.