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

String vs number ids and ember-data 5.3+ issues #485

Open
Techn1x opened this issue Jul 5, 2023 · 3 comments
Open

String vs number ids and ember-data 5.3+ issues #485

Techn1x opened this issue Jul 5, 2023 · 3 comments

Comments

@Techn1x
Copy link
Contributor

Techn1x commented Jul 5, 2023

I'd like to use string ids across the board, as that's what my JSONAPI does and it's also what ember-data generally expects.

EDFG seems to always use number ids by default when using a factory.

nextId() {
return this.modelId++;
}

That might have worked in the past, but I think ember-data is a bit more strict now. On ember-data 4.12.1, it seems that for a patch/update payload, if ED receives an id from the api (string) that does match match what is cached in its store (number), it fails with this error;

Error: Assertion Failed: Expected the ID received for the primary 'teacher' resource being saved to match the current id '1' but received '1'.
    at assert (index.js:153:1)
    at JSONAPICache.didCommit (index.js:627:103)
    at eval (index.js:432:22)
    at Store._run (index-757cf686.js:5403:5)
    at Store._join (index-757cf686.js:5419:12)
    at eval (index.js:429:11)

This can be replicated with code like the following;

const teach = make('teacher') // pushes a teacher to the store with id 1 (number)
// console.log(teach.id) // record in store has id '1' (string)
mockUpdate(this.teacher)
  .match({ locale: 'fr' })
  .returns({ attrs: { language: 'fr' } })
teach.name = "newname"
await teach.save()

The workaround is to set an id in the model when creating it make('teacher', { id: 'my-string-id' }) but I've got a lot of tests to update if I go that path..

I think the fix is to either force string ids from that nextId function, or give an option to users to set a flag in a factory to use string ids.

@Techn1x
Copy link
Contributor Author

Techn1x commented Jul 5, 2023

Looking deeper, it might be specifically something to do with the mockUpdate(...).match(...).returns(...) because it seems to work without the match/returns chains.

Either way, I still think giving the option to use string ids in factories would be beneficial

@Techn1x
Copy link
Contributor Author

Techn1x commented Nov 16, 2023

Also, I think string ids is be a requirement of ember-data 5 & 6.

It's a deprecation item here until ED6
https://deprecations.emberjs.com/ember-data/v5.x#toc_ember-data-deprecate-non-strict-id

We are deprecating this legacy support for numeric IDs.

But even when using ED with compatWith: '5.3' it will fail with

Error: Assertion Failed: Resource IDs must be a non-empty string or null. Received '1'.

@Techn1x Techn1x changed the title String vs number ids and ember-data issues String vs number ids and ember-data 5.3+ issues Nov 16, 2023
@esbanarango
Copy link

Plus 1

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

No branches or pull requests

2 participants