CC-Archived / promise-as3

Promises/A+ compliant implementation in ActionScript 3.0
168 stars 55 forks source link

"optionally" swallows parameters when target function uses ...rest parameter #33

Closed fwienber closed 10 years ago

fwienber commented 10 years ago

The utility function optionally uses Function#length to determine how many of the given parameters are actually handed through to the targetFunction. Unfortunately, functions that use a ...rest parameter have a length of 0 and thus do not receive any parameters, although they expect arbitrary many. optionally is used e.g. in Consequence#transform() to call the transformation function. The intention seems to be that functions without any parameters can be used, but I don't really see the use case (return a constant value?!). Can you give an example of a case where optionally is needed for a transformer function? A rather common use case is to define delegating functions that hand on all their parameters to another function. To implement generic delegation, you typically define a function with a ...rest parameter and apply the original function with the actual parameters, which makes you run into the bug: optionally considers the delegating function to have zero arguments and thus swallows all parameters.

johnyanarella commented 10 years ago

I tend to agree that attaching a Promise handler with no parameters is a bad smell. It typically indicates an API where the developer is using a Promise as a signal or trigger rather than its intended purpose of facilitating delivery and transformation of future values. It does happen, though, and there is nothing in the Promises specification that bars developers from doing so.

Without the logic you see in place there leveraging optionally(), if a developer does omit the expected parameter, an Error will be thrown, and that Error will trigger a rejection. This tends to vex developers because the Error does not originate in the body of the function they wrote, but rather from its signature, so they tend not to immediately grasp why things are going wrong. If the developer does not add a rejection handler or use .done(), the Promise will fail silently. This tends to be a major hurdle when people first start working with Promises.

Further, the API expectations for Promises have originated from JavaScript, where the rules are much more lax regarding expected parameters when calling functions. Originally, this behavior was not part of Promise-AS3 and was added specifically at the request of developers who were used to the JavaScript behavior.

Thanks for pointing out that particular quirk of ...rest functions. I did not anticipate that anyone would ever pass a function defined with ...rest parameters as a then() handler.

I'm torn on how to handle this particular concern. Pragmatically, the current flexibility is useful to consumers of this library. Pedantically, it is irksome that as a result of that flexibility, variadic functions are not supported as handlers.

Could you provide an example where generic delegation would be useful in this context? I honestly don't understand why a developer would do what you describe in ActionScript, since methods are bound to their instance (unlike JavaScript).

Why would you do:

promise.then( function ( ...rest ):* {
    return instance.method.apply( instance, arguments );
});

when you can do:

promise.then( instance.method );
fwienber commented 10 years ago

That would happen for any higher-order function that produces functions that can handle an arbitrary number of parameters. Take for example the well-known function curry that takes a function with n parameters and converts it into a function with one parameter that returns a function with n - 1 parameters:

function curry(fn:Function):Function {
  return function(a:*):Function {
    return function(...rest):* {
      return fn.apply(null, [a].concat(rest));
    }
  };
}

You can then for example take a method with two parameters, use curry to hard-wire the first parameter, and use the curried function that takes the remaining one parameter as a transformer function in Promise.map(). Assume obj has a method convert(flag:Boolean, value:String):String and promises is an array of promises of strings, then this should work:

Promise.map(promises, curry(obj.convert)(true));

optionally would consider the function resulting from curry to have zero parameters, so the string values would not arrive at the convert function. Since promise-based programming suggests a functional programming style, higher-order functions should be supported.

johnyanarella commented 10 years ago

Oh!

Thanks for the clear example. I appreciate your persistence on this issue.

Yes, I'm fully convinced this is a major problem now. I hadn't thought about how functional helpers like curry would typically produce delegates that expect ...rest parameters in ActionScript.

I wonder if it would be viable to optimistically assume the handler accepts a single parameter and only inspect Function#length and re-execute it when that particular Error is thrown. It would introduce some overhead for those who are omitting parameters, but at least it wouldn't suddenly stop working for them. Perhaps a warning might be in order within that particular catch block, too.

fwienber commented 10 years ago

Sounds like the perfect solution: only adds overhead for the hopefully rare use cases, keeps existing code working, and fixes the newly detected misbehavior. Thumbs up!