[PATCH] add Aspect capability

js + tests

Why "*Advice"? :-/

Replying to mislav:

Why "*Advice"? :-/

Well, from the information I gathered, in AOP, an advice is a function added to an insertion point (see wikipedia).

Most AOP implementions seem to use that terminology. There is also very little chance that it would conflict with the name of a method in the class it is extending...

How do you like passing the returned value (and original method) as an argument rather than as a property of this?

Cleaner? Worse?

Thanks for your feedback!

Looks good at first glance. I'm not sure about the verbose names -- avoiding a naming conflict is noble, but can be a problem with any mixin. We don't do it for Enumerable, so for consistency's sake (and for the sake of being Ruby-ish) I'd suggest keeping the original before, after, and around.

I like the idea of passing the original function rather than doing this.yield(), but I want to make sure there are no unforeseen side-effects. Perhaps we could get some other opinions to see which approach seems more intuitive.

Looks good at first glance. I'm not sure about the verbose names -- avoiding a naming conflict is noble, but can be a problem with any mixin. We don't do it for Enumerable, so for consistency's sake (and for the sake of being Ruby-ish) I'd suggest keeping the original before, after, and around.

I see your point. However, Aspect is probably not going to be used as the other mixins are. I believe it will often be added to pre-existing classes (extending script.aculo.us, for example) whose developpers will not necessarily have taken Aspect into account. Having to modify the name of a method so as to be able to extend the class it belongs too kinds of defeats the whole purpose. That's why I believe more verbose names are not such a bad thing...

I like the idea of passing the original function rather than doing this.yield(), but I want to make sure there are no unforeseen side-effects. Perhaps we could get some other opinions to see which approach seems more intuitive.

Sure. That's one of the biggest change this implementation brings. I'd like to have Beppu's opinion on it too but cannot contact him.

this.yield() seemed shaky to me, wouldn't it break if more then one around filters were added in a class? Shouldn't it be a hash of original methods rather then just the method itself ?

Did you have a chance to check the restoreOriginalMethod() method. I had to use a really ugly hack to make it work for methods that also are methods of Object (like toString()). Any other, more elegant suggestion would be really appreciated!

Looks interesting, and perhaps useful, but I wonder whether it should be part of Prototype. I like the Mootools approach of having plugins which handle concerns like this. Thoughts?

I like the idea of passing the original function rather than doing this.yield(), but I want to make sure there are no unforeseen side-effects. Perhaps we could get some other opinions to see which approach seems more intuitive.

I checked Dojo's implementation. They too pass the original function as an argument.

Actually, I can see three different ways of to handle passing the original function and its arguments:

1. The Dojo way: passing the original function and its own arguments as properties of an object:

   instance.addAroundAdvice(morph, function(invocation){
     invocation.args[0] += 'color: #300';
     return invocation.proceed(invocation.args); 
   }); // makes morph always morph to text color #300
   

2. passing the original function and its arguments as two distinct arguments, like this:

   instance.addAroundAdvice(morph, function(proceed, args){
     args[0] += 'color: #300';
     return proceed(args);
   }); // makes morph always morph to text color #300
   

3. the solution I originaly proposed (passing the original function as the first argument, concating its own arguments to it):

   instance.addAroundAdvice(morph, function(proceed, style, options){
     style += 'color: #300';
     return proceed(style, options);
   }); // makes morph always morph to text color #300
   

   It is easier to work with, but has the disadvantage of having to specifiy again all the arguments you want to pass to proceed().

Now that I thaught about this more, I'd go for the second solution. What do you think?

Replying to eventualbuddha:

Looks interesting, and perhaps useful, but I wonder whether it should be part of Prototype. I like the Mootools approach of having plugins which handle concerns like this. Thoughts?

That's an interesting point, but Prototype has not really been thaught out like that. I'm not really into that "let's save 10kb on JavaScript" craze myself. The whole of Prototype doesn't weigh more than an average picture, so I'm fine using it the way it is.

I dug into Dojo a wee bit deeper, Dojo's invocation.proceed() actually internally calls apply on the original method so you can do this:

instance.addAroundAdvice(morph, function(invocation){
  invocation.args[0] += 'color: #300';
  return invocation.proceed(); 
}); // makes morph always morph to text color #300

However, we agreed with savetheclocktower that

instance.addAroundAdvice(morph, function(proceed, style, options){
  style += 'color: #300';
  return proceed(style, options);
}); // makes morph always morph to text color #300

was a better solution. It's nicer to work with, closer to other Aspect implementation and it doesn't pass all the arguments by default.

We still don't really agree on how to name the methods of Aspect. the following have been suggested:

• before, after, and around,
• $before, $after, and $around,
• addBeforeAdvice, addAfterAdvice, and addAroundAdvice,
• addFunctionBefore, addFunctionAfter, and addFunctionAround.

There are some issues with restoreOriginalMethod() if more than one user starts adding advices to the same method, as what one user might expect as being the original method not necessarily is. Any suggestions on this issue are welcomed.

thanks get a cool air force ones