model id not being set in Store.prototype.create #145

Closed
pushkin- opened this Issue Jul 21, 2015 · 5 comments

Projects

None yet

2 participants

@pushkin-

In the function Store.prototype.create (line 138 in .js) there are the following lines of code: (some are omitted).

if (!model.id) {
    model.set(model.idAttribute, this.generateId());
}
...
this.records.push(model.id.toString());

So if the model doesn't have an id, one is set.

The problem is that model.id.toString() results in an error for me: Uncaught TypeError: Cannot read property 'toString' of null;

I set a breakpoint in the if statement.
In my case, I get there once, when model.id is undefined.

After the set call, my model.id is null, it doesn't seem to set the id.

I have some issues down the road that may or may not be due to this, but why is the id not being set?
If need be, let me know how I can help you reproduce this issue.

Edit:
I found the culprit.

In the switch statement in dualsync(), under 'update', model's id is being explicitly set to null. It looks like in options.success the id gets set back to what it was. In fact it's the same thing in options.error. Is it getting short circuited somewhere?

This issue occurs when I instantiate a model offline.

Edit 2:
It does appear to add my model to localstorage and then sync properly when I'm online, so nothing traumatic is going on. I am still curious as to why this happens.

Additional info:
/stuff_dirty is empty, so it synced
/stuffnull has the model I added: {"title":"test","id":null}
/stuffSERVERID also has that model: {"title":"test","id":"SERVERID"}

Should /stuffnull be there?

@nilbus nilbus added the Bug label Jul 27, 2015
@nilbus
Owner
nilbus commented Jul 27, 2015

Is it getting short circuited somewhere?

Apparently it is. This looks like a bug to me, present since the most recent release, 1.4.0 in 863db19.

    when 'update'
      if model.hasTempId()
        temporaryId = model.id

        options.success = (resp, _status, _xhr) ->
          # The following line was added later, not considering that it would leave the model's id missing.
          return useOfflineStorage() if hasOfflineStatusCode options.xhr
          updatedModel = modelUpdatedWithResponse model, resp
          model.set model.idAttribute, temporaryId, silent: true

I will fix this as soon as I free up, but if you can do it before then, that will be helpful since I'm pretty busy at the moment.

After the set call, my model.id is null, it doesn't seem to set the id… why is the id not being set?

That is also strange, because Model.set does not trigger a dualsync call that has the bug we're talking about. In the backbone.js annotated source for Model.set, backbone updates model.id when the idAttribute is set. What is your model's idAttribute set to?

@pushkin-

I didn't explicitly set idAttribute, but it defaults to id.

@nilbus
Owner
nilbus commented Jul 27, 2015

Okay. I'll look into this when I can. Thanks for bringing it up!

@pushkin-

An important piece of information that I stripped from my initial comment is that I'm using ReactJS with Backbone.

That is also strange, because Model.set does not trigger a dualsync call that has the bug we're talking about.

set doesn't trigger dualsync but it triggers a change event which may be the cause of my problems.

When I add an item, React's componentDidUpdate is triggered which in turn calls save -> dualSync. Eventually in Store.prototype.create, model.set() is called. set triggers a change which results in onComponentDidUpdate to be called again and the cycle repeats. save -> dualsync -> update...

I will investigate this more, but it seems like the issue is most likely in my code. My way of integrating with React seems to cause some problems.

Could you explain why id is being set to null in the switch.update in dualsync (it follows the code you posted)? Is that just so when it syncs, it fires a POST instead of PUT, and the item can be updated with an id from the server rather than the temporary one?

@nilbus
Owner
nilbus commented Aug 12, 2015

Could you explain why id is being set to null in the switch.update in dualsync (it follows the code you posted)? Is that just so when it syncs, it fires a POST instead of PUT, and the item can be updated with an id from the server rather than the temporary one?

Yes. See the annotated source below:

when 'update'
  if model.hasTempId()
    # model.hasTempId() indicates that the model was saved while offline.
    # Backbone triggered an update because it has an id,
    # but on the remote server, the model has not yet been created.

    # Save the tempId for restoring later
    temporaryId = model.id

    options.success = (resp, _status, _xhr) ->

      # If we're still offline, this will `return` and do a localsync `update`
      # without restoring the temporary id. Oops! Bug
      return useOfflineStorage() if hasOfflineStatusCode options.xhr

      # If online, this `updatedModel` will have the new id from the server
      updatedModel = modelUpdatedWithResponse model, resp

      # Delete the old model with the old id from local storage
      model.set model.idAttribute, temporaryId, silent: true
      localsync('delete', model, options)

      # Store the updatedModel with the new id in local storage
      localsync('create', updatedModel, options)

      success(resp, _status, _xhr)
    options.error = (xhr) ->
      model.set model.idAttribute, temporaryId, silent: true
      relayErrorCallback xhr

    model.set model.idAttribute, null, silent: true
    options.xhr = onlineSync('create', model, options)
@nilbus nilbus self-assigned this Aug 12, 2015
@nilbus nilbus closed this in d916965 Aug 12, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment