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

Default conversion _id to id #21

Closed
crobinson42 opened this issue Sep 16, 2016 · 3 comments
Closed

Default conversion _id to id #21

crobinson42 opened this issue Sep 16, 2016 · 3 comments
Assignees

Comments

@crobinson42
Copy link
Member

crobinson42 commented Sep 16, 2016

// Why couldn't Mongo just use "id"?

No shit?!

@jmdobry How do you feel about making default functionality to convert id to _id and vica versa with adapter interactions?

We can still have the config idAttribute property to be specified for override.

@jmdobry
Copy link
Member

jmdobry commented Sep 20, 2016

Hmm.

Pros Cons
Saves users from having to set idAttribute to _id in mapperDefaults The JSData schema will be slightly out of sync with how the data is stored. id vs _id
id is prettier than _id Might be surprising for users, as the rest of their app might already be using _id, and most MongoDB users know the pk field is _id
Might make it easier to combine the mongo adapter with other adapters that don't expect _id Could make debugging a little more difficult?

I think in the end, this might violate the Principle of Least Astonishment. It's simple enough to do

new Container({
  mapperDefaults: {
    idAttribute: '_id'
  }
})

I might be okay with adding an option that translates id to _id and visaversa, but I think it should be disabled by default.

This also raises the question of whether JSData's Mapper class should have some better facilities for mapping fields in memory to different fields in the database. i.e. camelCase to snake_case, id to _id, etc.

@crobinson42
Copy link
Member Author

Understood. I'll put together a PR that at least makes it an option to convert in the adapter - leaving the default behavior as-is.

@crobinson42
Copy link
Member Author

@jmdobry I'm sure you can point out improvements.. but this is the general idea #29

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

No branches or pull requests

2 participants