Open jbeezley opened 6 years ago
@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?
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.
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. 🤔
I was seeing strange issues where many methods on the
Model
class were not working correctly. As minimal example: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.