Breaking changes in 0.x release: "You cannot wrap null, undefined, or primitive values using Lazy"

[alexgorbatchev]

Thanks a ton for keeping up the good work! We've just pulled 0.2 release and found that I breaks many cases that we use.

The culprit is https://github.com/dtao/lazy.js/blob/master/lazy.js#L94 and the You cannot wrap null, undefined, or primitive values using Lazy error. This check makes API not compatible with 0.1 release.

Our general use pattern is like so:

results = MongoCollection.find(...)
results = lazy(results).do().stuff().toArray()

Often results would be null based on the find() query but because we used lazy 0.1 we would always have at least an empty array in the end.

0.2 breaks this pattern.

What are the thoughts behind it and is this a final call on the API?

[dtao]

First: awesome—thanks for the feedback. Without input from users, I'm basically just making decisions that seem sensible to me. And even though I give myself some slack when it comes to backwards compatibility—semver says anything can change as long as the major version is < 1—I do want to be as supportive as possible to any developers using Lazy already!

Second: no, it's not a final call on the API. I was sort of 50/50 on what should happen. It sounds like you would prefer for an empty sequence in this case, which I would be totally fine with.

What do you think? I can certainly revert back to returning an empty sequence. Based on your opinion here I can also easily release 0.2.1 shortly.

[alexgorbatchev]

@dtao Agreed on semver point.

I would prefer lazy to be more helful than not. On the surface it seems unnecessary for lazy to be strict about this but I would love to hear your reasoning behind this.

Perhaps there could be a lazy.strict flag that would go together with this change? The flag could be false by default or true if you wish to eventually move towards stricter implementation.

[dtao]

I'm not really opinionated about it. My reason for choosing to throw an error was basically that wrapping null wasn't really a legitimate scenario I had considered, and so I figured it's better to throw an error than have undefined behavior (because I didn't want to put any thought into how to handle that case).

That said, I can see how the opposite approach actually makes more sense. Because the truth is nobody's going to explicitly call Lazy(null); rather—as you've pointed out—you might pass Lazy(x), where x could be null.

I would prefer not to have a 'lazy.strict' option, for two reasons:

1. Introducing the precedent for global-level options is just going to take me down a rabbit hole of complicated behavior that I know will end up having bugs.
2. It seems to me that Lazy can just pick one way to do things, and then users can easily wrap that behavior if they would prefer to do things another way.

To elaborate on that second point: I suppose if Lazy just gives back an empty sequence if you pass in null, but some dev would rather have errors thrown, then he/she could just write something like:

var Lazy = (function(lazy) {

  return function(source) {
    if (source == null) { throw 'Do not wrap null or undefined with Lazy!'; }
    return lazy(source);
  };

}(require('lazy.js')));

(On the other hand, if we had the opposite scenario then users could just create a helper function that passes source || [] into Lazy instead.)

So it sort of feels to me like it doesn't make a big difference. I think that I'm going to revert back to having an empty sequence on null, since it does seem a bit more helpful like in the scenario you've already described.

[alexgorbatchev]

I like your second point.

Just tonight we had an hour long discussion at work about one of the open source libraries we maintain and the main point ended up being is that a helper library should do everything to reduce amount of work required for the user and be as helpful as it can be within its own domain.

Because of that thinking, my preference would be Lazy(x) over Lazy(x || []).

And as you said, you can always make you own wrapper to be stricter, but it's much harder to go the opposite way.

[dtao]

OK, I think I've arrived at a good solution here.

For Lazy's default behavior, I've switched back to the old way; so Lazy(null) will return an empty sequence.

For those who prefer a stricter implementation, they can do this:

var lazy = require('lazy.js').strict();

lazy()           // throws
lazy(null)       // throws
lazy([1, 2, 3])  // sequence: [1, 2, 3]
lazy.range(1, 4) // sequence: [1, 2, 3]

I realize this is quite similar to your flag idea; but in my mind it's preferable because... well, I guess I'm just paranoid about flags, is what it comes down to.

That said, I wanted to just point something out that occurred to me since yesterday, in response to your initial question of why a stricter implementation could be preferable. Of course it sounds pretty obvious to say: "libraries should be helpful and reduce the amount of work developers have to do." But I don't think it necessarily follows that in all circumstances it makes life easier for developers to return an empty sequence for null. What is helpful in one context might be the opposite in another.

As an example, suppose a dev were to write code like this:

function findMatches(pattern, count) {
  performExpensiveQuery(function(results) {
    var matches = Lazy(results).match(pattern).take(count);

    if (matches.isEmpty()) {
      console.log('No matches!');
      return;
    }

    matches.each(function(match) {
      console.log(match);
    }
  });
}

Now suppose there was code somewhere that called findMatches, and every time it ran the dev saw "No matches!" So he tries to figure out what's wrong with his regex, or investigates why the DB is returning no results, or something—without realizing that he just forgot to return a value somewhere, so results in undefined. In this scenario, it would have been more helpful to throw an error so the dev could have understood the problem immediately.

Anyway, I just wanted to present that counterexample, in response to the idea that this API choice is simply a matter of being helpful.

Of course, you could argue that I'm still not being helpful by putting the burden on developers of whether they want strict or lenient behavior... but oh well! Deal with it ;)

The old behavior will be back in 0.2.1, which I expect to publish today or tomorrow.

[alexgorbatchev]

@dtao You make a good argument about the null and I really like your final solution. Thank you very much for being flexible on this! :)

Would you mind pushing 0.2.1 out?

[dtao]

0.2.1 is pushed!

[alexgorbatchev]

very cool! thank you!

also, yesterday I noticed that .groupBy() doesn't work the same on hash tables :) but i found a workaround... please do a change log next time!

[dtao]

Yeah, I have one now as of 0.2.1: https://github.com/dtao/lazy.js/blob/master/CHANGES.md

Haha, sorry to keep breaking your stuff! Early adopter woes, eh? ;)

Could you let me know what you tried to do with groupBy that didn't work, and what your workaround was?