Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Call parse before retrieving idAttribute #198

Merged
merged 1 commit into from
Nov 3, 2012

Conversation

kopatsy
Copy link

@kopatsy kopatsy commented Sep 23, 2012

I have an API that returns the object payload as follow:
{
result : {
id : 123
field1 : ...
}
}

I have a custom parse method on my models that essentially does 'return response.result'.

In the findOrCreate method, the idAttribute must be retrieved and
it uses item[ type.prototype.idAttribute ]. This is fine when the
API returns the id as a top level key but it brakes otherwise and
parse must be called first.

I have not had a chance to write a unit test yet. I am new to javascript and backbone and I wanted to get your feedback on the change before I invest more time implementing a test case. This fix solves my particular problem but it may have side effects I am not aware of. Without this fix, I would get 'Cannot instantiate more than one Backbone.RelationalModel with the same id per type! ' errors since findOrCreate would never be able to pull the model idAttribute.

In the findOrCreate method, the idAttribute must be retrieved and
it uses item[ type.prototype.idAttribute ]. This is fine when the
API returns the id as a top level key but it brakes otherwise and
parse must be called first.
DouweM added a commit that referenced this pull request Nov 3, 2012
Call parse before retrieving idAttribute
@DouweM DouweM merged commit 4fcbb19 into PaulUithol:master Nov 3, 2012
@DouweM
Copy link
Collaborator

DouweM commented Nov 3, 2012

Looks good to me. I can't think of any unwanted side effects, and for a change like this extra tests aren't necessary if you look at the level at which existing tests for this project are defined.

Thanks for your contribution!

PS. Sorry for the late response, I've been awfully busy lately.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants