Add liftAll

[briancavalier]

As suggested by @phated, it seems quite convenient to lift all the methods of an object in one go, e.g. var fs = nodefn.liftAll(require('fs'));

Some questions in my mind:

1. Should it add new own methods to the original object, or should it beget? My instinct is to do the latter, leaving originals untouched.
2. How should it name these new, lifted methods?
   • Use the same name as the original, or
   • Use a new name with a standard suffix, like "-Async", eg fs.readFileAsync, or
   • Force/allow the caller to provide a naming strategy. This might be as easy as adding an optional param to liftAll: function liftAll(object, namingStrategy), where namingStrategy is a function string -> string, given the original name, return a new name. Perhaps it could also accept false (not falsy), meaning "use the original name".

It'd be easy to provide a liftAll variant in when/function, when/node/function, and when/callbacks ... lift allthethings.

[briancavalier]

Just pushed a quick and dirty take on when/node/function liftAll in 313af96. It begets, and allows a naming strategy function.

[briancavalier]

The when3-lift-all branch has liftAll variants for regular functions, node-style, and old-school callbacks.

@phated: It'd be great if you could give it a spin and let me know what you think. The node-style liftAll is require('when/node').liftAll. Currently, the namer function is optional:

1. If you pass your own naming function, it'll use that,
2. If you pass explicit false for namer, it'll use the identity function (ie lifted methods have the same name as the originals)
3. Otherwise, the defaultNamer is used, which simply appends "Async" to names

[phated]

Thanks! I hope to get that branch dropped in my app this evening

[jescalan]

Ohhh man this is an excellent feature, great idea. Will be using this one for sure!

[briancavalier]

It's still an open question in my mind as to whether namer is the right thing. Since liftAll uses Object.create, there's no risk of overwriting originals, and it seems like naming confusion (ie when you see fs.readFile(...) in code, is that the lifted version or the regular version?) can be mitigated by naming the newly generated host object intuitively, eg var fsAsync = liftAll(require('fs'));

Hmmm, thoughts?

[phated]

@briancavalier That's not the problem the namer is solving. I have to give a Redis client to my session object (specifically redsess) and it uses the standard redis methods, expecting them to take callbacks. If I overwrite those properties, I have to have 2 instances of a redis client, which is a pain.

[briancavalier]

Great, thanks for the use case, @phated!

A couple more issues, which we discussed on IRC, but I wanted to capture them here. It'd be great to get @unscriptable's thoughts on these, too.

1. liftAll(object) runs var newObject = Object.create(object) and attaches lifted methods to newObject. What should liftAll(aFunction) do? There is no analog to Object.create for functions. Some options:
   1. "copy" aFunction using bind or by wrapping it, then lift all its methods.
   2. mutate aFunction by adding properties to it directly. Creates an ugly situation: we beget from objects, but mutate functions.
   3. Disallow it and fail loudly. This is how the code in when3-lift-all behaves right now :)
2. Should var g = liftAll(f) (when f is a function) also lift f? That is, should g be a lifted version of f? What if f is a constructor? Read on ...
3. Does lifting constructors make any sense at all? I say no, since it means all lifted constructors return instanceof Promise not instanceof TheConstructor. That's bizarre: var x = new Thing(); x instanceof Thing === false; x instanceof Promise === true; WTF for sure.

[briancavalier]

Another issue: the current impl uses Object.keys which is safer, but not ideal for things like: liftAll(new Thing()) since it will skip all the prototype methods, which is probably not what you intended.

So, the question is, how should we handle this situation? Some possibilities:

1. Add an option for liftAll to lift inherited methods in addition to own methods.
2. Provide another function, liftAllInherited (or whatever name) that lifts inherited and own.
3. Don't support it. Using for..in in liftAll would change the own property landscape of the new Thing.

Any other options?

[scothis]

I'm a little late to the party on this one. I like the concept of liftAll, it's very similar to what I was trying to achieve with node-then https://github.com/node-then/node-then/blob/master/lib/node-then.js#L65. And it's application to 'fs' https://github.com/node-then/fs-then/blob/master/lib/fs.js.

The concept of a whitelist was useful, although any strategy could be used. Brining properties forward that were not lifted was also useful, as it made the new object api compatible.

The particular implementation uses when/node/function#bindCallback() instead of #lift(), as I was able to preserve back compat when a callback function is still provided.

I opted to create a new object rather than give lifted objects a new name. Either publishing the lifted modules as a new package (see fs-then), or lifting the modules as they are required into your script seems reasonable.

I do like that you beget the entire current object for the new object; I only begot the object's prototype.

[phated]

For many of my use cases (including Redis/RedSess), I would want for..in, but for others (fs, etc) I would want Object.keys. I would be fine with this being optin through an argument or another API (liftAllInherited).

[briancavalier]

@scothis Ah, eligible/whitelist is nice. On bringing props forward, yeah, you might be right, especially for own props of the original. Seems likely that using Object.create on the original will handle most situations, but it does change the own prop landscape if liftAll doesn't copy properties forward :/

Very interesting note on using bindCallback, too. Since liftAll renames by default, there's no conflict with the original methods, but if you decide to overwrite the originals (eg by passing false), I can see bindCallback being useful.

Maybe the right guiding principle for liftAll for 3.0 is: do the simplest thing that could possibly be useful, and add options/variants later as we encounter real-world use cases for them.

[unscriptable]

There's no way we can know everything folks will want to do with liftAll(), so I'm leaning towards the simplest possible thing like @briancavalier suggests. In short, I don't think we should provide namer or a filter. These seem like very simple operations to perform on the thing that is returned from liftAll()

[briancavalier]

This is a good point, @unscriptable: adding options, like namer is a slippery slope. For example, why don't we also provide an eligible parameter that allows you to do what @scothis's eligible does? Then, why don't we also add the ability to pass in your own object or function as the target to which we attach the newly lifted methods? And so on.

The simplest thing that can possibly work, imho is liftAll(x). We assume x is an object, run var y = Object.create(x) on it, lift all of x's methods under their original names onto y. One additional feature which doesn't feel too slippery to me is to allow x to be a function, which we copy by running y = x.bind(), then proceed as above.

Here's another thought that just occurred to me. We could potentially provide a generic liftAll:

function liftAll(liftOne, x) { ... }

// where liftOne is of the form:
function liftOne(originalName, functionToBeLifted) {
    // returns [newName, liftedFunction];
}

The specific liftAlls in when/node, when/callbacks, etc. could use the generic one internally, and the generic one could also be available to developers (via when/liftAll or somesuch) who want to run a customized liftAll.

[briancavalier]

Here's another possibility, based on discussion w/@phated about his use cases. liftAll is conceptually a fold (reduce), so maybe we provide a base liftAll that is a fold. Something like:

function liftAll(liftOne, object, combine) {
    if(typeof combine !== 'function') {
        combine = defaultCombine;
    }

    return Object.keys(object).reduce(function(lifted, key) {
        if(typeof api[key] === 'function') {
            return combine(lifted, liftOne(object[key]), key);
        }
        return lifted;
    }, Object.create(object));
}

function defaultCombine(o, f, k) {
    o[k] = f;
    return o;
}

The defaultCombine above overwrites, but it wouldn't have to--we can make the default whatever seems best. The point is: if we let callers provide the combine function, they can do whatever they need. Fits with my latest mantra: when you think you need a bunch of options, accept a function instead :)

If we wanted to take the analogy further, the base liftAll could also accept an "initial value", just like a fold. The "initial value" would be the target host to which lifted methods are added:

function liftAll(liftOne, src, combine, target) {
    if(typeof combine !== 'function') {
        combine = defaultCombine;
    }

    return Object.keys(src).reduce(function(lifted, key) {
        if(typeof api[key] === 'function') {
            return combine(lifted, liftOne(src[key]), key);
        }
        return lifted;
    }, target);
}

Keep in mind that this is a base version. From it, for example, when/node's liftAll can be derived easily: by partially applying when/node's lift function as liftOne.

[briancavalier]

I just force-pushed an update where the base liftAll is a parameterized fold. So far, I'm liking it.

[phated]

:+1: I like this implementation. I think it could go one step further and remove the defaultSuffix part if you wanted it to override the method by default.

[jescalan]

Just read through the whole thread, like these changes. So the only way to change the suffix would be by providing your own combine function?

[briancavalier]

@phated: Thanks! Yeah, I think that'd safe since by default, we're doing Object.create(src) or src.bind() internally when you don't specify a dst, ie you'd have to opt-in to clobbering by explicitly passing the same src and dst.

@jenius Yep, that's correct. The thinking is that we can't anticipate the options that every developer will ever need/want, so letting them pass a function is the only way the base liftAll can ever be "flexible enough".

[briancavalier]

This just landed in the dev branch, with unit tests for when/lib/liftAll (the generic base version). Unit tests for the when/node and when/callbacks versions coming soon. Feel free to give them a go tho!

[jescalan]

:confetti_ball:

[briancavalier]

@jenius you're the github emoji master!

[jescalan]

:whale: :dolphin: :fish: :sparkling_heart:

[briancavalier]

Calling this done. If we encounter any problems before release, we can open a new issue.