adopted-ember-addons / ember-data-factory-guy

Factories and helper functions for (unit, integration, acceptance) testing + development scenarios with Ember Data
MIT License
301 stars 136 forks source link

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

Open Techn1x opened 1 year ago

Techn1x commented 1 year ago

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. https://github.com/adopted-ember-addons/ember-data-factory-guy/blob/c4ccbaa70880084c13097c8c4ab6e4e1d6f5434b/addon/model-definition.js#L80-L82

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 commented 1 year ago

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 commented 10 months ago

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'.
esbanarango commented 4 months ago

Plus 1