cujojs / meld

AOP for JS with before, around, on, afterReturning, afterThrowing, after advice, and pointcuts
Other
644 stars 65 forks source link

Provide access to current joinpoint #8

Closed briancavalier closed 11 years ago

briancavalier commented 11 years ago

It can be useful to have access to the current joinpoint from within advice types other than around. It'd be nice to find a clean way of allowing that. Some AOP libs provide it as a "free variable" if the language/platform allows. Others allow it to be passed as a typed parameter (the AOP engine sniffs the param types at weave or run time). Neither of those seems feasible in JS.

Some ideas:

  1. meld.joinpoint() or somesuch method that returns the current joinpoint.
  2. require('meld/joinpoint') - similar, but a module (which might just be a function) that provides access to the current joinpoint.
  3. Change the meaning of this while inside advice functions. This seems horribly dangerous at this point and would probably require a major version bump. My least favorite option.

It's a toss-up between 1 and 2 for me right now. For 1, it's nice to only need to ever remember 1 module, but for 2, it's nice that an advice implementor can require a simpler module rather than meld itself.

Pinging @unscriptable and @scothis for thoughts and other options.

briancavalier commented 11 years ago

I prototyped option 1 in the joinpoint branch, and you can see how it's used in the unit tests.

Not too bad. Internally, it's implemented with a stack to ensure that you can still call advised functions within advices or within other advised functions, and you'll always get the "correct" joinpoint when you call meld.joinpoint(). You could certainly end up doing funky things if you try to hold onto the object that meld.joinpoint() returns, since it represents the current state of the call stack. IOW, whenever you call meld.joinpoint() what it actually returns is the top of the joinpoint stack, i.e. "the most-recently-entered joinpoint".

I haven't thought of a way to make it any safer. It could just be a documentation issue.

scothis commented 11 years ago

Options 1 and 2 are essentially the same, it's just where you stick the method. if .joinpoint() is part of the public API, it might as well be on the meld object.

In the long run, I'd lean towards option 3. Yes, it will change the public API semantics, there are other changes that should be considered as well, for example, after advice can not substitute an alternate return or thrown object. These changes were certainly necessitate a major version bump.

briancavalier commented 11 years ago

Yeah, turns out that 1 and 2 are the same--even the same number of characters! 2 can even be implemented on top of 1, and vice versa. Guess there's only 2 choices here :)

3 is def interesting, but it'll break existing aspects that depend on this. They'll need to use joinpoint.target instead. If there's enough need, we can look into it for 2.0.

AFAIK, it's typical for afterReturning and after not to be able to modify the result. I know Spring's AOP support behaves that way, but I'm not sure about pure AspectJ, do you know?

briancavalier commented 11 years ago

From some quick reading, Spring.NET is the same as Spring AOP, which probably should have been obvious, but just looking for how other libs behave ... I'll keep looking around.

briancavalier commented 11 years ago

Just found this thread which implies that AspectJ behaves similarly as well (after cannot change the return value).

briancavalier commented 11 years ago

After thinking about this a bit more, I believe option 1 is the "simplest thing that can possibly work" for the 1.x line. I think we can solve the "don't cache the joinpoint() return value and try to use it outside your advice function" problem through documentation and examples. I also don't think there's any realistic way to enforce it in JS.

briancavalier commented 11 years ago

Closed by d38ba7ad8a7b5506ab9f41988c0abf9e3495718b