null passed to set method as key argument causes problems

[dhritzkiv]

Upon updating my code to v.4.9.0, I started getting errors in set about converting undefined or null to object.

The error happens on L155:

//`attrs` is null, for some reason
//pass `null` as the argument to `Object.keys` blows stuff up
for (var i = 0, keys = Object.keys(attrs), len = keys.length; i < len; i++) {

A few lines higher, on L124, there's an explicit check for null, and the code also sets attrs to null:

 // Handle both `"key", value` and `{key: value}` -style arguments.
if (isObject(key) || key === null) {
    attrs = key;
    options = value;
} else {
    attrs = {};
    attrs[key] = value;
}

---

I'm not sure where in my code I'm calling model.set(null, …) (it could be happening internally or in another ampersand module), but I'm looking into tracking it down as soon as I submit this issue.

[wraithgar]

The old way of for (attr in attrs) would not throw on null or undefined.

[wraithgar]

This was changed in an optimizaton PR from @STRML https://github.com/AmpersandJS/ampersand-state/commit/c6a16ed689dea762b2b7c67368ae8c4456d506e7

Looks like an edge case not currently covered in tests got broken.

[dhritzkiv]

So I think I've managed to track down what conditions cause null to be passed as the key argument to set.

Say I have a model that contains another model as child property:

var User = State.extend({});

var Post = State.extend({
    children: {
        user: User
    }
});

Then I fetch some data from the server where user is null (intentionally)

{
   "user": null
}

Then it comes time to set the Post's user property with null

//L165
this[attr].set(newVal, options);//attr = "user", newVal = `null`

which is where things break.

---

This should demonstrate that null is a valid value to be passed to set when setting a child model. (or at least it had been working previously).

A quick fix might be to change Object.keys(attrs) to Object.keys(attrs || {}) on L155. This also demands another test case. I'm happy to submit a PR with the above fix and a test case for null

[STRML]

Gotcha - I'll add a failing test for this and fixup set().

[STRML]

Ah @dhritzkiv if you're willing to do a PR that'd be great - pretty busy here today - I appreciate it.

[dhritzkiv]

Yep. Can do.

[wraithgar]

Heads up there's a PR for this now from another contributor. Looking into why the travis tests are failing.

[wraithgar]

fixed in v4.9.1