Ruby on Rails | Screencasts | Download | Documentation | Weblog | Community | Source

Ticket #6810 (new enhancement)

Opened 2 years ago

Last modified 1 month ago

[PATCH] add Aspect capability

Reported by: Tobie Assigned to: sam
Priority: normal Milestone: 1.2.7
Component: Prototype Version: edge
Severity: normal Keywords: aspect
Cc: mislav, savetheclocktower

Description

This is based on Beppu's implementation.

It differs from the original mainly in the following ways:

  • it has to be applied as a mixin, either on a Class or an instance:
      Element.extend(MyClass.prototype, Aspect); // on a class
      Element.extend(instance, Aspect); // on an instance
    
  • it has a restoreOriginalMethod() method
  • after(), before() and around() have been replaced by addAfterAdvice(), AddBeforeAdvice(), and addAroundAdvice(). More verbose, but limits risks of name conflicts with the original methods of the extended class.
  • addAfterAdvice() passes the returned value of the orginal method as the first argument of the Advice (the added function), e.g.:
    instance.addAfterAdvice(toString, function(returnedValue){
      return returnedValue.toUpperCase();
    }); // changed the output of toString to uppercase.
    
  • addAroundAdvice() passes the original method itself as the first argument of the Advice, e.g.:
    instance.addAroundAdvice(show, function(proceed){
      if(Effect) return visualEffect('Appear');
      else return proceed();
    }); // uses scriptaculous effects if available.
    

It would be great to have your feedback on this proposed implementation in Prototype.

Attachments

aspect.diff (8.8 kB) - added by Tobie on 12/10/06 01:31:40.
js + tests

Change History

12/10/06 01:31:40 changed by Tobie

  • attachment aspect.diff added.

js + tests

12/10/06 01:32:35 changed by Tobie

  • keywords set to aspect.

(follow-up: ↓ 3 ) 12/10/06 11:09:45 changed by mislav

  • cc set to mislav.

Why "*Advice"? :-/

(in reply to: ↑ 2 ) 12/10/06 18:04:23 changed by Tobie

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!

(in reply to: ↑ description ; follow-up: ↓ 5 ) 12/11/06 03:08:07 changed by savetheclocktower

  • cc changed from mislav to mislav, savetheclocktower.

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.

(in reply to: ↑ 4 ) 12/11/06 03:40:48 changed by Tobie

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!

(in reply to: ↑ description ; follow-up: ↓ 8 ) 12/11/06 17:34:11 changed by 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?

(follow-up: ↓ 9 ) 12/11/06 23:24:35 changed by Tobie

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
    

  1. 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
    
  1. 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?

(in reply to: ↑ 6 ) 12/11/06 23:31:59 changed by Tobie

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.

(in reply to: ↑ 7 ) 12/13/06 23:50:48 changed by Tobie

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.

05/30/08 17:58:19 changed by airforce1