_.property support for null and undefined

[melnikov-s]

The _.property method will currently throw an error if the returned function is passed in a null or undefined value. With this PR, in such a case undefined will be returned instead. This seems to be the least surprising behaviour as opposed to crashing the program.

contrived example:

var predicate = _.compose( _.partial(_.isEqual, 'foo') , _.property('foo'), _.property('bar'));
var result = _.filter([{bar : {foo: 'foo'}}, {foo: 'foo'}], predicate); 
console.log(result); // => [{bar : {foo: 'foo'}}]  : where as it would crash before

[mahdi-shojaee]

👍 LGTM

A simpler use case can be:

var arrays = [[1, 2], null, [3]];
_.min(arrays, _.property('length')); // => [3]

[michaelficarra]

-1, people should handle null values explicitly.

[jdalton]

This change should extend to _.pluck and _.invoke too.

[davidchambers]

people should handle null values explicitly

It's not difficult in this case, either:

_.min(_.filter(arrays, Boolean), _.property('length'));

[jashkenas]

Unless there's a compelling use case for silently swallowing the error, Michael is right...

[jdalton]

Unless there's a compelling use case for silently swallowing the error, Michael is right...

I think Michael is actually wrong on this. Methods like _.extend, _.defaults, _.union skip over nullish values. Extra checks/work-arounds are code smell that Underscore helps to eliminate.

[melnikov-s]

What benefit do explicit null and undefined checks provide?

How many times do you see code that looks like this if (obj && obj[key]) just to prevent an error being raised. Or worse yet, as in backbone options || (options = {}); creating a an empty object just to prevent null/undefined checks.

[michaelficarra]

@jdalton: I think Michael would rather see an inconsistency here than have this function adopt the poor semantics of other underscore functions. I also think nobody's definition of right/wrong in this thread is objective, so we should avoid using those terms.

[jdalton]

Talking about yourself in the third person is odd ;)
I'm for consistency and savings devs grief, allowing errors on nullish fails both of those tenets.

[michaelficarra]

Devs can save themselves their own grief if they just pay attention to the types of the values they're working with. I'm not convinced underscore should gloss over it for them.

[jdalton]

I rely on the behavior of ignoring nullish values. It saves me extraneous checks and is a great convenience. I'm also aware of the potential types, usually null or some other object. Glossing over the empty values is a plus.

[braddunbar]

Devs can save themselves their own grief if they just pay attention to the types of the values they're working with.

This is ad hominem and not pertinent. The fact of the matter is that sometimes nullish values will sneak through. The question is whether or not undefined is an intuitive enough value when it does. I, for one, think it's a fine default.

[michaelficarra]

Not ad hominem because I am not trying to discredit @jdalton in order to discredit his argument. Instead, I am arguing that "saving devs grief" would be letting them know (via runtime TypeErrors) when they're working with types of data they didn't expect rather than hiding that fact from them until it surfaces somewhere distantly related.

[braddunbar]

I wasn't referring to @jdalton. I meant users of values that are sometimes null. You're discrediting their use of a sometimes null value to discredit undefined as a default return for nullish values.

[melnikov-s]

I am arguing that "saving devs grief" would be letting them know (via runtime TypeErrors) when they're working with types of data they didn't expect rather than hiding that fact from them until it surfaces somewhere distantly related.

Having _.property throw a TypeError is already distantly related to where the nullish value originated. Why is it the job of _.property to throw it?

It is similar to using _.map on an nullish value to map over all the properties of that value, obviously there are no properties on a nullish value so you get back an empty array. You also get back an empty array for running _.map on an empty object.

_.property should behave the same way.

[megawac]

👍 makes sense and is consistent with the rest of the lib

[bmar]

Gotta love having a record of these conversations. I had a similar problem years ago, and I was on the fuzzy null side of the argument. According to this convo, it seems the balkanised opinions on the matter still continued for many years (maybe even after this merge). Although too late to save the state of my app at the time, I'm glad that pluck can now handle jagged data.
bmar@ece0f5f