Lodash iterators break when adding a "length" attribute

[jbeezley]

I was seeing strange issues where many methods on the Model class were not working correctly. As minimal example:

class Test extends Model {
  defaults() {
    return { length: 0 };
  }
  validation() {
    return { length: positive };
  }
}

test = new Test();
// test.attributes is an empty object
// test.validate() is true

Looking into it more, I found that lodash iterators use the presence of the length attribute to determine if an object is an array or not. This causes all objects passed into lodash to be treated as an array.

I image I could override the rest calls to rename these properties coming from the server, but I'm wondering if there is an officially sanctioned way of dealing with reserved names, or if this is a problem that has not been considered.

[rtheunissen]

@jbeezley this is a problem that hasn't been considered exactly. There are some reserved names that would clash with internal naming, but the length interaction with lodash hasn't come up before, and it was only a matter of time until it did. We'll need to take a closer look at a proper solution here and write some tests to get around it. We may need to avoid lodash entirely?

[jbeezley]

I ended up fixing it in the constructor like this

class Test extends Model {
  constructor(attributes, ...args) {
    if (attributes && _.has(attributes, 'length')) {
      attributes.length_ = attributes.length;
      delete attributes.length;
    }
    super(attributes, ...args);
  }
}

This allows it to work with the fetch method on the Collection class, which is all I needed.

I don't think avoiding lodash will solve anything. There will always be property collisions when using bare objects. This one happens to cause particularly weird behavior. I would suggest to throw an error or warning when the attributes argument is detected as an array.

Ideally, there would be some sort of escape hatch available when the REST API contains reserved keys. Perhaps something like

class Model {
  rename() {
    return { length_: 'length' };
  }
}

where it would automagically rename the attributes when passing to and from the server.

[rtheunissen]

I think the only solution here is an accessor object for the attributes, much like we use $ for saved attributes. For example: model._.length or model.$length or something. This would be a massive BC break though, and would make things like collection.filter({length: 5}) impossible because the accessor on the base object won't be there. It's a balance between a nice API and a flexible architecture.

However, I don't think we necessarily need to throw the baby out with the bathwater. So far, it seems that length is the only attribute causing a real problem here. We can prefix all internal fields easily, but we can't expect anyone to change their responses to work with the library. For that reason, the scope of this issue is to resolve issues around length only.

There is a lot on the topic in this Underscore issue thread from 2015: https://github.com/jashkenas/underscore/issues/1590

What we need to do here is write some failing tests where length is an attribute and is set and accessed as expected. We'll need to document a warning that using length as an attribute name will break iteration of the model if you use functions like _.each (the key will be undefined).

That should be sufficient for now.

As an aside, I'm contemplating changing the saved shortcut from .$. to just a $ prefix (leaving the former in but deprecating for the time being. For example, model.name and model.$name, where the first would be active state and the second would be saved state. 🤔