iterators with null fail in 0.8.0 #80

Closed
tommymessbauer opened this Issue Oct 1, 2012 · 29 comments

Projects

None yet

3 participants

@tommymessbauer

In previous versions, you could attempt to iterate over a null

var foo = null;
_.each(foo, function(... ) { .. });

In 0.8.0, it now throws this error.

TypeError: Cannot read property 'length' of undefined

I think it is related to this change.

afeld/underscore@30f0d32#L3R957

@tommymessbauer

While I do not totally agree with this post, this is fine for an answer.

jashkenas/underscore#803

@jdalton
Member
jdalton commented Oct 1, 2012

Did you mean to post this to Lo-Dash (noticed you referencing Underscore code)? The issue is valid for Lo-Dash too so it's all good. I removed Lo-Dash's more thorough guards to be more inline with Underscore 1.4.0 though I am monitoring reaction to see if they need to come back.

@jsdnxx
jsdnxx commented Oct 2, 2012

@jdalton I upgraded one of my projects at work today from 0.5.2 (!) to 0.8.0 and ran into this issue. However, it didn't affect too many areas of our codebase, and those where it was an issue, we added an explicit fallback (eg, collection || []) which, imho, makes our code more readable / predictable / explicit. +1 for keeping the change.

@tommymessbauer

I did mean to post this in lodash (and should probably post in underscore as well). I feel pretty strongly that keeping the null check is a good idea. It was a nice convenience and we have a lot of code that relies on it (and coincidentally makes our code tighter).

However, convincing the underscore and lodash groups for convenience sake might be more trouble than it is worth. We just locked to 0.7.0 for now. My hope is that others chime in and feel the same way.

@jsdnxx
jsdnxx commented Oct 2, 2012

I agree with jashkenas here: jashkenas/underscore#803 (comment) (I noticed you were participating in that thread as well, @tommydudebreaux).

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

It makes it so much easier when _ semantics closely match ES5 Array natives.

@jdalton
Member
jdalton commented Oct 2, 2012

@tommydudebreaux

I did mean to post this in lodash (and should probably post in underscore as well). I feel pretty strongly that keeping the null check is a good idea. It was a nice convenience and we have a lot of code that relies on it (and coincidentally makes our code tighter).

Ok cool.

However, convincing the underscore and lodash groups for convenience sake might be more trouble than it is worth. We just locked to 0.7.0 for now. My hope is that others chime in and feel the same way.

Aww naw, hush that fuss ;)
One of the strengths of Lo-Dash is its iterator template. It's trivial to make a build that adds or removes the falsey guards and have it used by all iteration methods. I'll look into making it a build option.

@jdalton
Member
jdalton commented Oct 2, 2012

@jden I haven't relied on the falsey guards either but it is a compat issue so I could see addressing it in a compat build option or something (though that's more work).

@jdalton
Member
jdalton commented Oct 2, 2012

Based on dev feedback over Twitter it looks like devs want the falsey checks.

@jsdnxx
jsdnxx commented Oct 3, 2012

-1. I understand it's a fine line between keeping library users happy, but I think changing the available method signatures as a build option will lead to library fragmentation, where your application works differently even on the same version of a library dependency based on the build option, with no way to predict that at runtime. I think it would be better to offer the additional method signatures as an extension over a core of underscore. Modular build options is great, but I think it should only go down to the level of what functions are available - not the behavior of otherwise indistinguishable functions. In the mean time, not upgrading from 0.7.0 is a very viable alternative for projects who don't want to take the new method signatures with falsely checks removed.

@tommymessbauer

+1 to what jden said.

Fragmentation or an ambiguous implementation of the same interface is worse than just moving on without the null check.

@jdalton
Member
jdalton commented Oct 3, 2012

Ok folks, I thought about it and here's the plan.

The Underscore patch that caused all this fuss landed less than a week before Underscore's 1.4.0 bump as part of the 30+ issues I reported. The goal was to make a choice for consistency. Lo-Dash up to that point had chosen to make all methods allow falsey (more than the handful Underscore allowed) for consistency.

I don't want to alienate devs or make them wait to upgrade because of this issue. I would rather have a --back-compat build option to allow it.

One of Lo-Dash's strengths is its build tool. I already use it to create a lodash underscore build that replaces our deep _.clone with Underscore's shallow counterpart, removes object / string iteration fixes, and removes the exit early feature of our _.each.

@tommymessbauer

Sounds awesome.

Which version do you plan to use in package.json? We are using browserify so our node and client versions are the same.. (obviously, I vote the falsey tolerant one)

If there is anything that I can help with, please let me know. I'll fork and send you the pull request.

@jdalton
Member
jdalton commented Oct 3, 2012

@tommydudebreaux I'm going with Underscore 1.4.0 compat by default and allow the --back-compat option for devs that need it.

@tommymessbauer

One other option could be to expose a lodash factory method.. It would mean be a code change, but might work for long term.

This would be current

var _ = require('lodash');

But you have the options of this..

var _ = require('lodash').create({ falsey: true });
@jdalton
Member
jdalton commented Oct 3, 2012

@tommydudebreaux It's technically doable on the Node side because of vm.runInContext but I'm gonna keep the build separate. You could always create a custom build and then add it to your node_modules folder.

@jdalton
Member
jdalton commented Oct 3, 2012

@tommydudebreaux Or you could use require('child_process').spawn and the build utility lodash underscore --back-compat --stdout and vm.runInContext(...)... Gaa too... much... geeking... out :P

@tommymessbauer

Yeah.. so if it takes a build, then I'll just migrate our code to do the checks externally. Right now, the only build steps in the entire stack are compass (yeah, less could eliminate that but we are using compass)..

The options in my view would be to

  1. fork and maintain the falsey version in npm
  2. upgrade my stuff and do the checks externally
  3. Stay on 0.7.0

I am going to opt for #3 for short term and hope the underscore folks change their mind. Eventually, the solution is #2. Forking in #1 just seems like a dick move to me.

-T

@jdalton
Member
jdalton commented Oct 3, 2012

@tommydudebreaux What's wrong with a custom build? It's not like you have to pre-compile your code. You build Lo-Dash once (it creates a lodash.custom.jsand a lodash.custom.min.js) and you can do with it what you want.

@tommymessbauer

The child process would work, but I have this weird aversion to runtime code generators. . (I also hold my keys extra tight when I walk over drainage grates)

I really like where your head is going with here.. However, I am really trying to stick with vanilla npm dependencies across the board. Running a build is something I have to sleep on and think about.

@jdalton
Member
jdalton commented Oct 3, 2012

@tommydudebreaux Ok welp. I can provide the options, if ya don't use em, ya don't use em.

@tommymessbauer

Oh snap.. What about adding a script to package.json that offered a way to build the other version from npm syntax.

npm install lodash
cd node_modules/lodash
npm compatibility

Then that could be tested and maintained in the package.

now I am totally geeking out too.. :)

@tommymessbauer tommymessbauer reopened this Oct 3, 2012
@tommymessbauer

oops.. did not mean to re-open.. ack!

@jdalton
Member
jdalton commented Oct 3, 2012

Oh snap.. What about adding a script to package.json that offered a way to build the other version from npm syntax.

npm install lodash
cd node_modules/lodash
npm compatibility

Well with a custom build you can do

npm install -g lodash
cd node_modules/
mkdir lodash
lodash --back-compat -d -o ./lodash.js
@tommymessbauer

Thanks for all of the options and being so responsive. I really appreciate it. For the near term, we locked to 0.7.0 because that is the easiest path.

(related to @jden 's comment earlier) One thing to think about is fragmentation.. once you support the compatibility mode, it is part of the library. While I really like the falsey feature, it might be best to omit it and keep it simple.

I am going to sleep on the options. We do not have much code (cause we keep things tight) so upgrading is not a big of a deal. In a perfect world, we all would agree and the underscore guys get enough pressure to restore that feature... (see how my perfect world is for them to come around to my view.. the irony is not lost on me)..

...and I wasn't joking.. I literally need some sleep.. :)

-T

@jsdnxx
jsdnxx commented Oct 3, 2012

I have no strong inherent feelings on tolerating falsey values, but jashkenas's feelings on basing the interface on ES5 native array functions makes sense to me. I have a medium sized code base (and lots of smaller things) at work that takes a dependency on lodash, and there was exactly one place that was affected by this interface change. It went from (to paraphrase off the top of my head) _.map(collection.slice(index)[0], fn) to _.map(collection.slice(index)[0] || [], fn)

I don't agree with how underscore went about deprecating an interface without prior warning in this instance - and we're seeing a fair amount of buzz around this on the issues board as a result. I think trying to create an interface-compatible library in javascript is difficult under the best of circumstances, and even more so when the interface is changing on a whim. There's tension between being faithful to the source interface (and in this case, I think there is obviously great value in this, especially given the popularity of Backbone) and doing right by API consumers. I still feel like providing incompatible APIs in the same version number - whether exposed through custom build processes or whatever other version - is a flawed approach. A possible approach to make the upgrade path easier would be a parallel 0.7.x release that doesn't break falsey collections in iterator functions, allowing devs more time to migrate their code bases.

@jdalton
Member
jdalton commented Oct 3, 2012

but jashkenas's feelings on basing the interface on ES5 native array functions makes sense to me.

Ya, he'ill use ES5 as a point when it's convenient in aiding to close an issue.
For example _.map and other methods allow not passing a callback and instead will use _.identity.

It went from (to paraphrase off the top of my head) _.map(collection.slice(index)[0], fn) to _.map(collection.slice(index)[0] || [], fn)

Yap, that's annoying to have to do. I just remembered a use case with string.match() which sold me on falsey checks initially. Related to issues #23 and #24.

There's tension between being faithful to the source interface...

Ok. Let me see if adding the checks breaks any existing Underscore unit tests :)

@jdalton
Member
jdalton commented Oct 3, 2012

Reverted here. I will version bump tonight. and bumped here.

@tommymessbauer

I'll unapologetically speak for loads of people in saying - "thank you"

@jdalton
Member
jdalton commented Oct 5, 2012

@tommydudebreaux Thanks for brining the issue to my attention. If you want this to land in Underscore you should continue the discussion over at underscore#744.

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