id attribute should not need to be placed in props

[mmacaula]

Consider this:

var State = require('ampersand-state');
var Model = State.extend({
  idAttribute : 'id', // not really necessary
});
var model = new Model({
  id : 'foo'
});
console.log("id is ", model.getId());  // prints 'id is undefined'

It seems a violation of DRY to have to include id in props when I specify it elsewhere in the definition. Backbone does this well IMO, I imagine it was not included in State for a reason, but am wondering if it makes sense to put it in now?

[kamilogorek]

If we do that, it'll currently break isNew function.

[mmacaula]

I don't understand. I thought models were 'new' if they didn't have an id. If an id was not included in the props sent to the model on creation, then isNew would still return true. If it were, then I would argue that the model is not new. Can you explain how it would break it?

Starting a tangent, should this functionality even be in 'state', would it make more sense to put it in ampersand-model?

[kamilogorek]

Model is new until it gets synced with some kind of storage using sync, which on the other hand will give a model it's own id. Current implementation of isNew checks for presence of attributeId defined attribute. If you'll pass and id to newly created model which is for example in the collection, and you'll want to send only new models to the backend, then it'll break, as isNew will return false even though you didn't store it yet.

[mmacaula]

Model should be new only if it has an id (that's what the source says). It shouldn't matter if sync has been called at all.

Even if it get's synced though, if I didn't define id in props, the result of sync won't set the id (I'm assuming for simplification that we're not using a custom idAttribute). That would make isNew return true, which brings us back to the original point.

[HenrikJoreteg]

Hi @mmacaula :)

Model should be new only if it has an id (that's what the source says). It shouldn't matter if sync has been called at all.

isNew is true when it doesn't yet have an ID. The idea is that you'd create an object in the clientside code and then use create to save it to the server collection.create({name: 'something'}) you're not going to create an ID for it in the client, that's something the server will provide when it's saved successfully.

The only time you'd set idAttribute if you wanted it to be something other than the string "id".

The reason we have it in state not model is because it's quite common to want to put state objects into a collection and collections use that idAttribute to figure out how it can determine if it already has a model with that id or not.

It also makes sense for developers to have to define the id attribute is to be able to see it listed as a thing alongside the other server properties reflected in the model. That way there's a very clear connection between the JSON coming from the server and the props on the model.

In addition, there are times when it's a number and times it's a string and being able to set its type is also useful for readability as well as some built-in error checking.

If we simply made id a property by default, everywhere then anytime you serialized a state object it would include: {id: undefined} which doesn't seem like desired behavior.

Anyway, these are the reasons it's done the way it is. Hope I described it ok. Feel free to re-open if this doesn't make sense. Thanks!

[mmacaula]

Thanks @HenrikJoreteg for the detailed reply! You're right, I got that wrong about isNew, must have fat-fingered it.

Perhaps I'm still thinking in Backbone on this, but I'm in the middle of converting over a backbone app and I have a lot of nested models that get fetched up front. Those models all still have ids from the server, but I'm using children and collections to manage them. I was just caught off-guard that I had to define the idAttribute of _id both in idAttribute (ours is mongo backend) AND in props. If we change our id attribute, now I have to change it in two spots. To me, it still violates DRY.

My proposal wouldn't prevent users from specifying the id in props, (and therefore getting type safety and readability). To me this is just favoring convention over configuration. If you want to define your id in props then nothing stops you, but if you don't you get smaller models and a default behavior.

If we simply made id a property by default, everywhere then anytime you serialized a state object it would include: {id: undefined} which doesn't seem like desired behavior.

That is a good point, but one that could be handled in the serialize or getAttributes methods if desired.

If you still disagree, that's fine (you know better than I!), perhaps I'd recommend updated documentation to make it explicit. For example the ampersand-model docs have lots of example models defined but none of them have an id in props. Coming from Backbone, that was cool and expected, but there's a bug waiting to happen in that code when the server does come back with an id and it gets ignored.

Anyway, those are my thoughts in a little more detail. Thanks for reading, I'll leave the issue closed unless you have been swayed by my amazing arguments! :smiley: