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

Non integer id broken with mutation #62

Closed
mkaay opened this issue Oct 11, 2018 · 6 comments
Closed

Non integer id broken with mutation #62

mkaay opened this issue Oct 11, 2018 · 6 comments
Labels
enhancement New feature or request help wanted Extra attention is needed
Milestone

Comments

@mkaay
Copy link

mkaay commented Oct 11, 2018

If a mutation returns a non integer id (like a relay node), a new record with id NaN is created.
I found this while debugging:

newData.id = parseInt(newData.id, 10);

Something like this might be better:

const newId = Number.parseInt(newData.id, 10);
if (!Number.isNaN(newId)) {
    newData.id = newId;
}
@phortx
Copy link
Collaborator

phortx commented Oct 12, 2018

Thanks for this issue, Marius,

The problem here is, that Vuex-ORM only supports integer IDs as far as I know and thus this plugin expects integer IDs too.

@kiaking Can you say something about the state of non-integer IDs for Vuex-ORM records? I think there was some issue when not using increment type as ID.

@phortx phortx added the enhancement New feature or request label Oct 12, 2018
@kiaking
Copy link
Member

kiaking commented Oct 12, 2018

@phortx Vuex ORM don't have any limitation on the value of ID (value of primary key). it can be string or number. or even null or boolean as well (I don't think you should though).

So I think there's no problem storing the id value as string. In fact, Vuex ORM will convert the primary key value to string if it's number, for the key of the state.

For example;

// If you save this data.
const data = {
  id: 1 // <- Number
}

// It becomes like this inside the state.
{
  '1': { // <- Key is the ID value and it's always string.
    id: 1
  }
}

If the ID value is string, it becomes like this.

const data = {
  id: 'string-id-001'
}

// Inside state.
{
  'string-id-001': {
    id: 'string-id-001'
  }
}

You are correct about this.increment() attribute only supports number (because otherwise string can't be "incremented"). But other than this.increment(), it's completely OK to pass string as a value.

@phortx
Copy link
Collaborator

phortx commented Oct 12, 2018

Ok, that sounds good. Thanks for the reply!

I will try to move to string based IDs soon, however PRs are welcome :)

@phortx phortx added this to the 1.0.0 milestone Oct 12, 2018
@fardarter
Copy link

@kiaking I was successfully using string based ids until I had to set up a pivot table. It sets up fine with ints but not strings.

Will be digging into it more this weekend. Hopefully I'm wrong.

I want ID overwrites for some of them. It's a series of defined roles; enumeration.

@phortx
Copy link
Collaborator

phortx commented Apr 27, 2019

I've setup a PR to make this pluggin support string based IDs. And all tests pass except the ones for hasManyThrough (pivot table) relations. I think there might by a bug in the core :)

#91

@phortx phortx added the help wanted Extra attention is needed label Apr 30, 2019
@phortx phortx mentioned this issue May 15, 2019
10 tasks
@phortx
Copy link
Collaborator

phortx commented May 21, 2019

Should be working with the next release.

@phortx phortx closed this as completed May 21, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request help wanted Extra attention is needed
Projects
None yet
Development

No branches or pull requests

4 participants