LeaVerou / bliss

Blissful JavaScript
http://blissfuljs.com
MIT License
2.39k stars 104 forks source link

Helper to manage function arguments #69

Closed LeaVerou closed 8 years ago

LeaVerou commented 8 years ago

This is a very frequent pattern in Bliss: A function which accepts either separate arguments or objects which provide the same values en masse. Here are a few examples:

Also, several others that might benefit from similar handling, e.g. $.events().

I was thinking, it would be nice to have a small helper to handle these cases, so that the functions themselves are only defined for the case of having everything as separate argument, and the helper takes care of looping if the argument is an object instead. It doesn't have to handle nested cases of this, since that only happens in $.delegate(), and we could apply the helper twice there. That way, not only the Bliss code will be simplified, but also our users get a helper function that might help them on their own code too. I spent some time thinking about this before releasing Bliss, but couldn't come up with a good idea for a good API for such a helper. Let’s brainstorm in this thread if you think it's worthwhile!

dperrymorrow commented 8 years ago

If I am understanding you correctly, underscore does something like this with an internal function to process arguments for functions that support "overloading".

https://github.com/jashkenas/underscore/blob/master/underscore.js#L105

Is that what you are getting at? Or something different?

LeaVerou commented 8 years ago

No, not at all, although what I'm referring to is indeed (a subset of) overloading. What I meant was, there are many functions that have two signatures:

function (key, value)
function (obj) // Multiple key-value pairs

and the implementation is one of the following two general themes (with many variations, especially on the checks):

// Object centric approach
function foo(obj) {
    if (arguments.length == 2) {
        obj = {};
        obj[arguments[0]] = arguments[1];
    }

    for (var key in obj) {
        value = obj[key];
        // [actual function logic]
    }
}
// Argument centric approach
function foo(key, value) {
    if (arguments.length == 2) {
        // [actual function logic]
    }
    else {
        for (var key in obj) {
            foo(key, obj[key]);
        }
    }
}
dperrymorrow commented 8 years ago

ok, I see, thanks for explaining further. I did work with this, i have some tests around this feature for $.lazy

Ill think about this.

brushed commented 8 years ago

What about using an overload helper function, taking care of the for...in loop ?

function overloadObjectArgument( foo ){

    return function( key_or_object, value ){

        var obj = ( arguments.length == 2 ) ? { key_or_object : value } : key_or_object;

        for( var key in obj ){

            foo( key, obj[key] ); 

        }
    };
}

var foo1 = overloadObjectArgument( function(key, value){
    // actual function logic
})();
var foo2 = overloadObjectArgument( function(key, value){
    // actual function logic
})();
dperrymorrow commented 8 years ago

yeah something like that, or perhaps not a callback function and it just takes the args and returns an object;

lazy: function(obj, property, getter) {
  var keyVal = processArgs(arguments);
  for(var key in keyVal) {
    // ...
  }
}

I'm working to finish the tests around the mentioned methods above so that when this is refactored to use a helper we would have coverage to ensure its working as expected.

LeaVerou commented 8 years ago

Nice work, everybody! I would rather use the argument-centric approach in the actual functions, instead of repeating the for..in loop every time. What @brushed suggested is closer to what I had in mind (we need to bikeshed the name though, I strongly favor names with only 1 word). One issue is that there aren't always 2 arguments, sometimes there are other arguments before those, so it probably needs an index that defaults to the case where they are the only arguments. Also, it needs to be possible to apply it twice, to work for things like $.delegate(). Another potential issue is what happens with the return value. I think all the methods that use it though are void, in which case it's not a problem.

dperrymorrow commented 8 years ago

ok so,

LeaVerou commented 8 years ago

No 3 is not as liberal as that. Usually the arguments before it are of a fixed number, so we could provide an index as an argument. Also, while I like one word names, it seems very difficult for something like this. But let's refrain from superLongJavaStyleNames() :)

brushed commented 8 years ago

We can cope with a variable number of fixed arguments by using "foo.length" which returns the declared number of arguments, and compare it to the passed arguments.

I've used map() in case of multiple invocations, so it's possible to capture the return values, if needed. FFS: this may not be smart e.g. in case of chainable invocations.

Unfortunately, the current implementation of $.add() cannot be generated with this helper, because in this case the foldable arguments are followed by 2 more optional arguments. Considering the complexity of $.add, I would suggest to simplify the implementation and only support objects as first parameter.

//unfoldArgs(fn) 
//    Returns a function which accepts a variable number of fixed parameters, 
//    followed by either separate key/value arguments or object arguments.
//    In the latter case, the function iterates over all key/value pairs.
//    Nesting of objects is allowed.
//    The return values are mapped into a result array.
//
//  supports foo(x, y ,z, key, value)
//  supports foo(x, y, z, { key: value}) -> foo(x, y, z, key, value)
//  supports foo(x, y, {z: { key: value} }) -> foo(x, y, z, {key: value}) -> foo(x, y, z, key, value) 
function unfoldArgs( foo ){

    //unfold the rest object; recursive.
    function unfold(nest, args, rest){

        if( (nest > 0)  && ($.type(rest) === "object") ){   

            return Object.keys(rest).map( function(key){
                return unfold(nest - 1, args.concat(key), rest[key] );
            });

       } else {

            if(rest !== undefined) args.push(rest);
            return foo.apply(this, args );

        }
    }

    return function( /*args*/ ){

        var args = Array.from( arguments ), //[].slice.apply(arguments),
             //foo.length = number of declared arguments on foo
            nest = foo.length - args.length,  //nesting level
            rest = nest > 0 ? args.pop() : undefined;  

        return unfold( nest, args, rest );
    }
}

Example for use in Bliss:

//object = $.lazy(object, property, getter)
//object = $.lazy(object, properties)
var $.lazy = unfoldArgs( function(object, property, getter){ ... });

//subject = subject._.delegate(type, selector, callback)
//subject = subject._.delegate(type, selectorsToCallbacks)
//subject = subject._.delegate(typesToSelectorsToCallbacks)
var $.delegate = unfoldArgs( function(type, selector, callback){ ... });

//$.live = function(object, property, descriptor)
//$.live = function(object, properties)
var $.live = unfoldArgs( function(object,properties){ ... });

//subject = subject._.set(property, value)
//subject = subject._.set(options)
var $.set = unfoldArgs( function(property, value){ ... });

//subject = subject._.once(type, callback)
//subject = subject._.once(typeToCallbacks)
var $.once = unfoldArgs( function(type, callback){ ... });

//!!NOK, cause the variable argument is not the last parameter !
//$.add = function (name, callback [, on])
//$.add = function (callbacks [, on])
//$.add = function (methods, on, noOverwrite)
var $.add = ???

Examples how this could be used / tested :

function test(a,b,c, key, value){
    console.log( "a:"+a,"b:"+b,"c:"+c,"key:"+key,"value:"+value, arguments)
}
var test1 = unfoldArgs( test );

console.log("****test1");
test1( 3, 4, 5, "key", "value");  //invokes test() once
test1( 3, 4, 5, { key1: "value1", key2: "value2" }); //invokes test() twice

console.log("****test2");
test1( 3, 4, "type", "key", "value");   //invokes test() once
test1( 3, 4, "type", {key1: "value1", key2: "value2"});   //invokes test() twice
test1( 3, 4, {type1: {key1: "value1", key2: "value2"}, 
              type2: {key1: "value1", key2: "value2"}});   //invokes test() four times

console.log("****error cases")
test1( 3, 4, "type", "xyz");  //invokes test() once with 4 parameters
test1( 3, 5, "type", 1234567); //invokes test() once with 4 parameters
test1( 3, 4, "type", true); //invokes test() once with 4 parameters
test1( 3, 4, "type", [1,2,3]);  //invokes test() once with 4 parameters 
test1( 3, 4, "type", function(a){ return true;} );  //invokes test() once with 4 parameters
test1( 3, 4, "type", "key", "value", "too much");  //invokes test() once with 5 parameters
dperrymorrow commented 8 years ago

I am finishing tests for $.live now and then i feel we have the test coverage needed to tackle this. As long as all the tests all check for the different parameter situations like this test. https://github.com/LeaVerou/bliss/blob/gh-pages/tests/objects/LazySpec.js#L86-L101 thanks for all the input @brushed , much appreciated.

brushed commented 8 years ago

I made a few improvements to my last post.
Still a few corner-cases to be looked at. ( chainable function or map() )

LeaVerou commented 8 years ago

Wouldn't it solve the $.add() case and improve performance if we just passed an index and a number of levels (1 by default) to the function?

dperrymorrow commented 8 years ago

hey all, just finished up tests for $.live i think Im at a good place to tackle this. Ill submit a pull request soon with the initial attempt at this.

dperrymorrow commented 8 years ago

working on this now, possible function names

LeaVerou commented 8 years ago

condensedArgs? overload? multiplied? enmasse?

I kinda like overload, though it's overly generic. Doubt we'll ever add anything for more overloading though.

dperrymorrow commented 8 years ago

cool, I have live working (with tests passing) with the helper method, the implementation looks like this.

live: function(obj, prop, desc) {
  $.overload(arguments, function (property, descriptor) {
    // code for $.live
  });

  return obj;
}

and the helper method looks like thus so far, pretty simple, but may have to add a bit more when i get to some of the more complicated methods. It takes an index param that defaults to 1 like you suggested.

overload: function(args, callback, index) {
  index = index || 1;
  var name = args[index], value = args[index + 1];

  if (!value) {
    for (var key in name) {
      callback(key, name[key]);
    }
  }
  else {
    callback(name, value);
  }
}

any feedback is much appreciated.

LeaVerou commented 8 years ago

Looks nice so far!!

LeaVerou commented 8 years ago

although, I think this would fail in cases like $.add() where the collapsible arguments are not last.

dperrymorrow commented 8 years ago

yeah ill have to sort something out for that to be more flexible. Ill burn that bridge when i get to it as they say...

LeaVerou commented 8 years ago

A more flexible check (e.g. is the first argument a string or an object?) would do, I think.

LeaVerou commented 8 years ago

Also, I was thinking of something that would wrap the function we passed to it. That way, there's no need to pass argument objects around. The wrapper function will call the callback via apply, after adjusting the arguments array on every iteration.

brushed commented 8 years ago

One more iteration of the unfoldArg() helper function; now also allowing a different position of the foldable argument.

unfoldArg() generates a wrapper function, unfolding any Object argument into key-value pairs. By default the argument to be unfolded is the last argument, you can give a position parameter to overwrite the default. Depending on the signature of the main function, multiple levels of unfolding will happen. (eg. needed for $.delegate() ) The wrapper function is also chainable.

//Helper function to unfold any Object argument into key-value pairs
//Arguments:
//  foo: the function with one unfoldable arg
//  pos: position of the foldable argument counting from the end of the argument list (default=1)
function unfoldArg(foo, pos){

    return function( /* args */ ){

        var args = Array.from(arguments), //[].slice.apply(arguments),
            nest = foo.length - args.length,  //foo.length = number of declared arguments on foo
            arg =  args.length - ( pos === undefined ?  1 : pos ),
            head = args.slice(0, arg),
            tail = args.slice(arg + 1);

        ( function unfold(nest, arg, keys){

            if( (nest > 0)  && ($.type(arg) === "object") ){

                for( var key in arg ){

                    unfold( nest-1, arg[key], keys.concat(key) );

                }

            } else {

                foo.apply(this, head.concat(keys, arg, tail) );

            }
        } )( nest, args[arg], [] );

        return this; //make chainable
    }
}

Usage :

//object = $.lazy(object, property, getter)
//object = $.lazy(object, properties)
var $.lazy = unfoldArg( function(object, property, getter){ ... });

//subject = subject._.delegate(type, selector, callback)
//subject = subject._.delegate(type, selectorsToCallbacks)
//subject = subject._.delegate(typesToSelectorsToCallbacks)
var $.delegate = unfoldArg( function(type, selector, callback){ ... });

//$.add = function (name, callback, on, noOverwrite)
//$.add = function (callbacks, on, noOverwrite)
var $.add = unfoldArg(function(name,callback,on,noOverwrite){ ...}, 3);
dperrymorrow commented 8 years ago

sure thing, ill wrap it, I now have tests passing on $.lazy, $.live, and $.set

@brushed thanks

LeaVerou commented 8 years ago

@brushed I like how this does everything automatically, but:

  1. We don't want to unfold any object argument. There are tons of functions that accept objects as arguments with various options, and we don't want those unfolded.
  2. To manage to do everything automatically, it ends up requiring much more code than @dperrymorrow’s solution and the recursiveness, while elegant, also makes it confusing.

I think providing an index and depth, with reasonable defaults, is the more understandable and simpler solution, even if less elegant.

brushed commented 8 years ago

unfoldArg(..) does not unfold any object argument, only the one indexed with the 'pos' argument, or, default, the last argument. While you could add a depth parameter (default 1) this can easily deduced from the argument list of foo; comparing it to the actual nummber of passed arguments.

brushed commented 8 years ago

The complexity really comes from the fact we'd like to support multiple depths. If we'd limit that (practically probably only 2 levels would ever be used) we could avoid the recursion and still keep an elegant solution.

LeaVerou commented 8 years ago

Sorry for missing pos, I didn't have time to understand the code in detail, I just skimmed over it. I’m a bit skeptical about automatically determining the number of levels. Just because the values are objects does not necessarily mean they should be unfolded, and there is currently no way to protect against that. Using the argument list of foo is also a bit flimsy: What if foo has other optional arguments, which are handled internally by the passed function?

I would much rather provide the number of maximum levels (defaults to 1) and keep things simple & predictable. I love heuristics too, but they can be dangerous when there’s no way out.

Btw, to be clear, big thumbs up for this code. It’s very clever and elegant, even if not 100% practical in this case!

brushed commented 8 years ago

Just to be clear, the number of levels is detected based on the number of arguments in the signature of foo versus the number of arguments passed when calling the wrapped function.

But indeed this means that optional arguments (eg such as in $.add()) are not ok.

LeaVerou commented 8 years ago

Exactly. Like I said, I love heuristics too, but good heuristics fulfill two conditions:

  1. The number of use cases where they fail is tiny
  2. Opt-out is possible for those use cases.
LeaVerou commented 8 years ago

Since we now have $.overload(), I’m closing this. We do need docs for it though...