Closed patocallaghan closed 3 years ago
As an extra data point to the stability of these changes I've ran these changes against the Intercom test suite, which has thousands of tests and makes heavy use of Factory Guy and Model Fragments, and CI is green 🟢.
Fix for #417
Background
While Factory Guy's primary use case is to generate Ember Data models it also supports the ability to generate Ember Data Model Fragments. Since Ember Data 3.13+ though there have been a number of compatibility issues between Model Fragments & Ember Data which have been outlined in detail in https://github.com/adopted-ember-addons/ember-data-model-fragments/issues/338. Previously trying to bump Ember Data in this repo resulted in all tests containing fragments to fail. At this stage the Model Fragments compatibility issues have been largely resolved with a big internal refactor to use
RecordData
instead (see changelog) so I think it's time we try and upgrade the Ember Data again.The problem
One issue we're still seeing though though is how Factory Guy generates Fragments using
make
. Themake
helper allows you to push records directly into the store rather than need to use thepush
,pushPayload
orcreateRecord
APIs to bootstrap your models. A common pattern when usingmake
is to compose your test models with other models usingmake
. For example take the followingperson
Ember Data model which contains a fragment:This can be created in a test by doing the following (once you've created your factories correctly). Here we create a user model and pass it an
info
fragment model.In Ember Data 3.16+ it appears passing a fragment to a model like above causes the properties of
info
to beundefined
rather than the values which were originally on the model.In
FixtureConverter
we usestore.push
to create the models. If you follow the calls down the internals it appearsstore.push
accepted any fragment instance as-is and just set it as a property on the model. At some point in Ember Data 3.16+store.push
was hardened to only accept JSON (which in fairness is what its public API states). I think it was a "happy accident" the fact it actually accepted a generated model fragment before and "just worked".This was pretty much confirmed by @runspired on Discord
Solution
When generating fixtures which are passed fragments (or fragment arrays) as properties we must pass their serialized form to
store.push
rather than passing them as instances.What is the breaking change?
The reason I"m calling this out as a breaking change is in some circumstances this will cause tests to fail if you have test assertions being run on the external fragment passed to
make
. The external fragment and the one generated on the model are no longer the same object.While this is a breaking change I'm also working on rolling up a bunch of other changes into the v4 release (See #444)