when.map(...) with two parameters and node.lift(...) issue

[nadvorny]

When I lift node function with two parameters e.g. fs.readFile(file, opts, callback) lifted function can't be directly used in when.map, because map's mapper function is called with two parameters, and array index is passed into opts parameter. I have to wrap it into a lambda to prevent second parameter from passing.

Lifting with partially applied parameters won't work because I need to apply or ignore the second one. Using lodash's _.partialRight(...) does not work either, because of how it works.

Ideally I would like to be able not to check if parameters should be frozen or not. It seems that deferred library (https://github.com/medikoo/deferred) has a syntax for lift function that sets function arity, e.g. promisify(fs.readFile, 1) will ensure that lifted function will have only one parameter, and other ones will be ignored.

var when = require('when');
var node = require('when/node');
var _ = require('lodash');

// readFile has the same signature as fs.loadFile
function readFile(param1, param2, callback) {
  console.log(Array.prototype.slice.call(arguments));
  callback(null, [param1, param2]);
}

var foo = node.lift(readFile);

var bar = function (fileName, index) {
  return node.lift(readFile)(fileName);
};

when.map(['1', '2'], bar).done(); // works
when.map(['3', '4'], foo).done(); // does not work

[briancavalier]

Hey @nadvorny, yeah, this is an interesting issue. Originally, when.map didn't pass the index. Users felt it was important to match that particular behavior of Array.prototype.map, which is a very reasonable expectation, so we added it.

I like your idea of a lift variant that supports specifying a max arity. Currently, we can't merge that functionality directly into lift, since it allows partialing (e.g. lift(readFile, 'filename') creates a lifted and partially applied function). However, that partialing is deprecated and will be removed in 4.0, so in 4.0 we could potentially allow an optional arity parameter.

Before 4.0, one option might be to add liftArity or liftn or somesuch that has a required arity parameter. Then, when 4.0 lands we could either: 1) add the optional parameter to lift, and deprecate liftArity, or 2) keep both to avoid optional parameters (lately, I don't like optional params).

Any thoughts on that? Other ideas?

Would like to get @unscriptable's thoughts as well.

[briancavalier]

Ping @nadvorny, any thoughts?

[nadvorny]

Actually I don't know which solution is the best for the library.
Different people will be using it differently. It would be really nice to cover all possible cases in sane and structured manner.

Correct solution in my case is to use _.curryRight() function on lifted function to freeze second argument.
This will transform function to a one argument function.
And _.curryRight should not be part of when library. It's a part of next lodash release.
Arity restriction will work on nodejs fs.readFile only because it's written so there are default options
and they work for my case. It may not work for other functions or if encoding needs to be specified.
Arity restriction now looks like a corner case to me.

There is one more problem with lifting a method of an object. Promise/A+ strictly states that
this should point to this from the promise creation time. So lifting methods of an objects doesn't work.
E.g. with couchdb (cradle) reading from database won't work:

var db = new cradle.Connection(...)
var readCouchDb = node.lift(db.view); // will fail
var readCouchDb = node.lift(db.view.bind(db)); // works

Adding when.lift_something() functions to the library may cover these cases,
but it will add unnecessary complexity into promise language.

May be these use cases should be just described in API documentation.

On 2015-01-26, at 05:13, Brian Cavalier notifications@github.com wrote:

Ping @nadvorny https://github.com/nadvorny, any thoughts?

—
Reply to this email directly or view it on GitHub #420 (comment).

[briancavalier]

It would be really nice to cover all possible cases in sane and structured manner.

I agree. In fact, that's what I thought you were proposing when you suggested a solution similar to deferred's ability to limit arity. Do you feel like a lift variant that explicitly sets max arity is not a good solution?

Another possible solution would be to remove the index parameter from when.map and provide a new operation when.mapWithIndex that passes the index. That'd be a breaking change, so we'd have to wait for 4.0. Another variant of that would be when.map(array, f, provideIndex=true), ie an optional parameter that specifies whether to call f with the index or not. I'm not crazy about the latter, but I figured I'd put it out for discussion.

There is one more problem with lifting a method of an object. Promise/A+ strictly states that
this should point to this from the promise creation time

That's not quite true. Promises/A+ says that this must be the default for handlers passed to then(). It doesn't deal with lifting at all, and so places no restrictions on this of lifted functions.

With respect to this, when.js lifted functions work just like normal functions. You can attach them to objects, or use call or apply to specify an explicit this. For example:

var db = new cradle.Connection(...);
// Obviously will not work:
var readCouchDb = db.view;
readCouchDb(...); // fails

// Also will not work, for the same reason as above:
readCouchDb = node.lift(db.view);
readCouchDb(...); // fails

// Works correctly:
readCouchDb = db.view;
readCouchDb.call(db, ...); // works

// Also works
readCouchDb = node.lift(db.view);
readCouchDb.call(db, ...); // works

That said, in general, yes, lifting instance methods is tricky business, but not specifically because of anything when.js does in particular.

[nadvorny]

With lodash 3.0 release and _.curryRight() being there this isn't a problem any more.

Adding arity only to protect from when.map index passing seems excessive to me.
Lambda will work ok in this case.
Adding arity to node.lift will require adding arity to all other lift functions to maintain.
Too much changes for not so used option.
Maintaining uniformity between when.map and Array.map seems reasonable to me.

On this and method lifting, I don't blame when.js in particular, I'm only learning promises now so I'm talking about
issues I had while learning to use them. Something that's not clear from documentation.
When you master promises and when.js implementation it becomes obvious why
functions and methods behave differently. Same with when.map.

So we don't need arity with curryRight is there.
We should update documentation to show how to use when.map with function like fs.readFile and node.lift with methods.
This should be enough.

On 2015-01-26, at 15:45, Brian Cavalier notifications@github.com wrote:

It would be really nice to cover all possible cases in sane and structured manner.

I agree. In fact, that's what I thought you were proposing when you suggested a solution similar to deferred's ability to limit arity. Do you feel like a lift variant that explicitly sets max arity is not a good solution?

Another possible solution would be to remove the index parameter from when.map and provide a new operation when.mapWithIndex that passes the index. That'd be a breaking change, so we'd have to wait for 4.0. Another variant of that would be when.map(array, f, provideIndex=true), ie an optional parameter that specifies whether to call f with the index or not. I'm not crazy about the latter, but I figured I'd put it out for discussion.

There is one more problem with lifting a method of an object. Promise/A+ strictly states that
this should point to this from the promise creation time

That's not quite true. Promises/A+ says that this must be the default for handlers passed to then(). It doesn't deal with lifting at all, and so places no restrictions on this of lifted functions.

With respect to this, when.js lifted functions work just like normal functions. You can attach them to objects, or use call or apply to specify an explicit this. For example:

var db = new cradle.Connection(...);
// Obviously will not work:
var readCouchDb = db.view;
readCouchDb(...); // fails

// Also will not work, for the same reason as above:
readCouchDb = node.lift(db.view);
readCouchDb(...); // fails

// Works correctly:
readCouchDb = db.view;
readCouchDb.call(db, ...); // works

// Also works
readCouchDb = node.lift(db.view);
readCouchDb.call(db, ...); // works
That said, in general, yes, lifting instance methods is tricky business, but not specifically because of anything when.js does in particular.

—
Reply to this email directly or view it on GitHub #420 (comment).

[briancavalier]

With lodash 3.0 release and _.curryRight() being there this isn't a problem any more.

Cool, that's great news.

Adding arity only to protect from when.map index passing seems excessive to me.
Lambda will work ok in this case.

Agreed.

I'll close this issue, and if we hit other situations where limiting arity is useful, we can revisit this discussion.