inconsistent treatment of null/undefined in _.defaults and _.extend

[davidchambers]

In a pull request on another project, I wrote:

_.defaults treats null/undefined as an empty object

I then realized I needed to add an exception:

… for arguments other than the first, that is

Why is this?

> _.extend(null, null)
null
> _.extend({}, null)
{}
> _.extend({a: 1}, null)
{a: 1}
> _.extend(null, {})
null
> _.extend(null, {a: 1})
TypeError: Cannot set property 'a' of null

The last two inputs above are particularly worrying. The current handling of _.extend(null, {}) is inconsistent and should be changed. I can think of two options:

• stick with the current approach of treating null/undefined specially in first position, but throw a TypeError in all cases rather than relying on a TypeError to be thrown in some cases; or
• treat null/undefined as an empty object in all positions.

I favour the latter, as it makes these functions easier to explain. We can then say "null/undefined arguments are treated as empty objects". Also, the functions will then have one return type rather than three (Object | Null | Undefined).

[davidchambers]

Related:

> _.extend(0, {})
0
> _.extend('', {})
''
> _.extend(NaN, {})
NaN
> _.extend(false, {})
false

[akre54]

For what it's worth, Backbone tends to use a guard on arguments that may be falsey, e.g. _.extend(options || {}, { validate: true })). I don't think that's particularly unreasonable here.

[davidchambers]

This is an anti-pattern, in my view. It's certainly frowned upon in Python, where one often writes if x is not None rather than if x, since the latter is shorthand for…

if not (x == False or x == None or x == 0 or x == [] or ...):

which is almost certainly not the thought one means to express.

[akre54]

But even an empty object is truthy in javascript. In python an empty dict or list is falsey.

If we know we're expecting an Object and not, say, a string (which could be '') or a number (which could be 0) then this shouldn't matter much.

[davidchambers]

If we know we're expecting an Object and not, say, a string (which could be '') or a number (which could be 0) then this shouldn't matter much.

The problem is that if we get an unexpected type we may misclassify it. For example, when we receive 0 we think we received null. Types should be validated before values.

[akre54]

But if you design your API right, you should rarely, if ever, need to do runtime type checking. _.extend is undefined for numeric types the same reason _.intersection &c is.

[braddunbar]

I've always thought of _.extend as returning it's subject exactly, regardless of it's type. I'd be fine with defaulting nullish arguments to {} though. I agree that we should treat _.extend(null, {}) and _.extend(null, {a: 1}) consistently.

[akre54]

So what's the solution, move the guard to inside the _.extend body then?

diff --git a/underscore.js b/underscore.js
index d25f5ca..8b18dfb 100644
--- a/underscore.js
+++ b/underscore.js
@@ -841,6 +841,7 @@

   // Extend a given object with all the properties in passed-in object(s).
   _.extend = function(obj) {
+    if (obj == null) return {};
     _.each(slice.call(arguments, 1), function(source) {
       if (source) {
         for (var prop in source) {

[jdalton]

The way to handle it is like other Underscore methods and not error on null or undefined and just let it fall through. The secondary arguments simply ignore null and undefined which is find, I don't believe the existing guard of if (source) { is needed as it's handled by the language with for (var prop in source) automatically.

So the desired behavior would be:

// so with a patch this following would
_.extend(null, {a: 1});
//=> null

// just as this currently does
_.extend(null, {});
// => null

[jdalton]

For what it's worth, Backbone tends to use a guard on arguments that may be falsey, e.g.
_.extend(options || {}, { validate: true }))

In the past I've handled if options was potentially falsey by adding them as secondary params:

_.extend({}, options, { 'validate': true });

This also has the benefit of not augmenting the passed in options object which may be reused in other areas.

[jdalton]

This also has the benefit of not augmenting the passed in options object which may be reused in other areas.

[akre54]

But it lets us avoid cloning the object if we don't care about mutating
options (which most the time we don't)

[jdalton]

But it lets us avoid cloning the object if we don't care about mutating options (which most the time we don't)

Most of the time? Underscore and Lo-Dash guard against it and I try not to augment user provided options objects in my own code. If you want to augment the provided options and guard against them being nulish then the existing _.extend(options||{}, ...) or _.extend(Object(options), ...) is fine.

Introducing your patch muddies the API in much the same way as passing function arguments to _.result.

The fix to avoid erroring and simply falling through aligns with the rest of the Underscore API without introducing other complications/blurriness.

[davidchambers]

I'm interested in reading what @jashkenas and @michaelficarra think of this issue.

[michaelficarra]

@davidchambers: While I don't particularly like it, @jdalton is right: null should just fall through, like all other non-objects, as you pointed out.

[davidchambers]

Thanks, Michael. I was actually trying to make a different point with that comment: I was under the impression that a logical false subject was given special treatment. I now realize this is not the case.

I'll put together a PR to treat a null/undefined subject the same as any other non-object, as suggested by @jdalton.