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

Already on GitHub? Sign in to your account

Use 'id' as key #143

Closed
wants to merge 5 commits into
from

Conversation

Projects
None yet
5 participants

ruiwen commented Jan 12, 2013

Adding the ability to use 'id' as the record key instead of manually specifying 'key' before save()'ing.

The rationale is that if I'm receiving data from the backend server, it's more likely that the primary key is named 'id' than it is named 'key', so this patch handles that.

I've only patched the adapters for memory, dom, webkit-sql and indexeddb though.

Tests included

Contributor

stigtsp commented Jan 29, 2013

I think this behavior should be added by a plugin, or be a configurable option instead, as it can yield unexpected results if you are not aware of the change of behaviour.

ruiwen commented Jan 29, 2013

If you're not aware of the change, the old 'key' format will still work though. And if you have 'id' set, it'll still be set as your 'key' field when you retrieve the records.

Contributor

stigtsp commented Jan 29, 2013

Hey! :)

Yeah I agree that it might be more convenient in that use case scenario, but the patch also introduces some subtle problems:

a) Existing usage might rely on 'id' not being used over 'key' (as it could be in the saved object by 'mistake', or if 'id' is used for other things). This could result in breakage that can be difficult to debug.
b) Both 'id' and 'key' will be duplicated in the data structure with the same value, and it also adds complexity to save() and get() for all adapters.

Why not instead check out the awesome callback.js plugin? It seems like you can easily create a hook for this and similar corner cases

store.before('save', function(r){if(r.id)r.key=r.id});

Updated the unit tests for callback.js to describe this functionality:

stigtsp/lawnchair@8fd4052

Contributor

stigtsp commented Jan 29, 2013

For the sake of completeness, it seems like the same concern was raised some time ago as well:

#20

Hi All,

Changing the behaviour of storing based on id and not key (even if there would be a key attirbute) is kinda strange.
When syncing data from couchdb objects will have an _id field. The one I map to the key when data arrives. It would be a lot nicer to make it configurable like described in issue #20, make the keyfield configurable for your store and default it to 'key' so it won't impact other users currently using key and maybe having an id field too.

ruiwen commented Feb 6, 2013

That's true, the key field should be a configurable (instead of a magic) option.

I'll see if I have some time to get round to trying my hand at this soon

Any updates on this? The configurable keyName would be helpful when working with a variety of server APIs. Could the code from issue #20 be merged in?

Collaborator

MarkMYoung commented Feb 23, 2017

This feature is addressed by the 'keyPath' plugin.

@MarkMYoung MarkMYoung closed this Feb 23, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment