1.4.0 breaks _each on empty objects or arrays #803

Closed
SirUli opened this Issue Sep 28, 2012 · 17 comments

Comments

Projects
None yet

SirUli commented Sep 28, 2012

Hello,

before version 1.4.0 _each has simply skipped the execution if the Array/Object was empty or did not exist. After 1.4.0 _each shows the following error if this situation occurs:

TypeError: Cannot read property 'forEach' of undefined
    at Function._.each._.forEach (/foo/bar/baz/node_modules/underscore/underscore.js:77:29)

If intended, this should probably be noted in the release notes. If not, this should be fixed? Workaround is to have a check for _.isUndefined before the execution of _.each.

Best Regards,
Uli

This only seems to happen when using Underscore in Node.js. In the browser, calling _.each with an empty object does not raise an error.

EDIT: It does in fact raise an error if it is called with null or undefined, but not for strings, numbers or empty arrays/objects.

Owner

jashkenas commented Sep 28, 2012

I've just added a note to the changelog about this -- but it's a desired breaking change. Previously, this happened:

_.map(null, _.identity)
=> []

... now you get an early error instead.

@jashkenas jashkenas closed this Sep 28, 2012

SirUli commented Sep 28, 2012

Thanks, that clears that up.

@tommymessbauer tommymessbauer referenced this issue in lodash/lodash Oct 1, 2012

Closed

iterators with null fail in 0.8.0 #80

idosela commented Oct 2, 2012

@jashkenas Can you please explain why you decided to change this behavior in v.1.4.0?
It seems useful to be able to call iteration functions without having to null-check every single time.

I have a large project which heavily relies on the old behavior, and is now not compatible with the latest version of underscore.

I know I can use _.mixin() to add null-checking back, but I am hoping there is a better solution.

Thanks for the great new features in v1.4.0

Cheers,

Ido

Owner

jashkenas commented Oct 2, 2012

Sure.

In general, in JavaScript (we're not in Objective-C), when you try to call methods on a null value, you get errors.

var list = null;

list.forEach(...)
list.map(...)
list.filter(...)

... and you want to have those be errors. If they silently did nothing, that would make the root cause of the error more difficult to find. For the same reasons:

var list = null;

_.forEach(list, ...)
_.map(list, ...)
_.filter(list, ...)

... should tend to error.

I agree completely with idosela here.

I think that removing the null check from the underscore iterators is a really bad idea. We have a bunch of code that has relied on this in underscore and it now has to be changed.

Underscore is a convenience library on top of javascript so trying to make it more like javascript (i.e. using javascript conventions) starts to make underscore a facade for javascript.. That starts to erode the value out of underscore.

So I feel pretty strongly on this, but in the grand scheme of things, we can lock to the previous version and upgrade at a later date (or obviously fork)

Thoughts?

idosela commented Oct 2, 2012

While I understand your point, I don't agree with your comparison.
The reason list.forEach() returns an error is because it is a method on a null object.
_.each() is a method of the "underscore" object (not of the list object), and as such has the advantage of being able to do null-checking (among other things).

I also don't agree that you want to have these errors; that's what good unit tests are for.

While I can work around it, I suspect this will be an issue for a lot of people.

bishopZ commented Oct 2, 2012

I strongly agree. Nulls should silently be discarded.

••• Sent Mobile •••

On Oct 2, 2012, at 12:25 PM, idosela notifications@github.com wrote:

While I understand your point, I don't agree with your comparison.
The reason list.forEach() returns an error is because it is a method on a null object.
_.each() is a method of the "underscore" object (not of the list object), and as such has the advantage of being able to do null-checking (among other things).

I also don't agree that you want to have these errors; that's what good unit tests are for.

While I can work around it, I suspect this will be an issue for a lot of people.


Reply to this email directly or view it on GitHub.

shanimal commented Oct 2, 2012

I strongly agree. Nulls should silently be discarded.

consider for(var i in list). Does it fail or ignore?

var list = null;
for(var i in list){
console.log("\t",i,list[i]);
}

EDIT : If $(".foo") is empty its still not null. So I'm silently discarding my petition.

Thanks again @jashkenas for an awesome library.

I'm with Jeremy even though it was an eye opener on Monday for one of my libraries. It was my fault for not one checking and / or two handling my code in a cleaner way.

It is not anyone's fault for not checking. Up until now, this was a feature. I am pretty sure that everyone specifically tested the functionality and thought "woah, that is convenient. I can write less code"

with the latest version, this is likely the tightest way to do this...

_.each( (myvar || []), function(val) { ... });

while that is not nice, this leads to even more code...

// pick your favorite array or object type checking hack.. 
if (myvar && myvar.length) { 
  _.each(myvar, function(val) { .. });
}

The original design was awesome. Why not stick to it?

It seems like the catalyst for doing this was this example:

_.map(null, _.identity)
=> []

Instead of pulling the check, why not return null in this case instead of empty? It seems like a completely reasonable way to handle it. Garbage in, Garbage out. Null in, Null out.

_.map(null, _.identity)
=> null

js-n commented Oct 3, 2012

Because

> Array.prototype.map.call(null, function () {})
<- TypeError: Array.prototype.map called on null or undefined

Not to say that ES5 got everything right, but sticking to the language-native API is neater.

It's actually fairly hard to end up with a null value in JavaScript accidentally (that's going to be one of those sentences I regret). I agree, @tommydudebreaux, that I like being able to write (confident code)[http://avdi.org/talks/confident-code-railsconf-2011/] without a lot of precondition checks everywhere. But my mind is not very good at keeping multiple things in the air at a time, and best not to confuse it with inconsistent behavior for seemingly identical interfaces (between underscore and ES5 native).

Shot in the dark here (and not to beat a dead horse [additional gratuitous cliche]), but maybe the _() function is an appropriate place to swallow that check and substitute an empty array if a falsey value is supplied? eg:

var a = undefined;
_(a).map(function () { return 'YAY!'; });
// returns ['YAY!']

Advantages:

  • the curried map function has a distinct interface from ES5's map, avoiding confusion. The behavior of underscore is clearly an extension, and therefore the library is free to make it's own decisions around what should happen with given input types.
  • the falsey check is limited to one place, which feels nice if you're chaining or composing larger functional expressions, avoiding doing the same work more than needed.

Disadvantages:

  • yet another thing (more complexity?)
  • still introduces a breaking interface change form previous versions, potentially requiring library consumers to update code.

@jden: There is at least one rather common way to end up with a null value in JavaScript: String.prototype.match

I use this combined with Underscore's each fairly often, like so:

_.each(someString.match(someRegex), someFunction);

Unfortunately, with the new version of Underscore, I can't use this if there is even the slightest chance that the regexp won't match. Fortunately, I don't really have to update. ;-)

Of course, vanilla JavaScript behaves that way when trying to call methods on a null value, but if I wanted vanilla JavaScript I wouldn't be using Underscore.js.

On the other hand, it is true that failing early is better than covering errors.

I'm still not sure if I like the changed behaviour or not.

js-n commented Oct 3, 2012

@indieboy86 so how would you feel about _(someString.match(/something/)).each(someFunction) ?

@jden Yes, why not. Still better than _.each(someString.match(someRegex) || [], someFunction);

buerkle commented Oct 3, 2012

Returning null will not remove the need to check for a null return value. I much preferred the original behavior. Most of the time a null/undefined value for a collection is the same as an empty collection. It fixes an inconsistency with the Array.prototype functions for other falsey values which return empty arrays.

Array.prototype.map.call(false, fn) => []
Array.prototype.map.call(0, fn) => []
Contributor

jdalton commented Oct 5, 2012

The discussion has been moved to #744.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment