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

[BREAKING] Fixes incompatibility with Ember Data Model Fragments 5.0.0-beta+ and Ember Data 3.13+ #449

Conversation

patocallaghan
Copy link
Collaborator

@patocallaghan patocallaghan commented Oct 4, 2021

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 adopted-ember-addons/ember-data-model-fragments#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. The make helper allows you to push records directly into the store rather than need to use the push, pushPayload or createRecord APIs to bootstrap your models. A common pattern when using make is to compose your test models with other models using make. For example take the following person Ember Data model which contains a fragment:

// models/user.js
export default class User extends Model {
  @fragment('info') info
}

// models/info.js
export default class Info extends Fragment {
  @attr name
}

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.

let user = make('user', {
  info: make('info'),
});

In Ember Data 3.16+ it appears passing a fragment to a model like above causes the properties of info to be undefined rather than the values which were originally on the model.

console.log(info.name); //=> "Jon Snow" 
console.log(user.info.name); //=> undefined

In FixtureConverter we use store.push to create the models. If you follow the calls down the internals it appears store.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
image

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.

let info = make('info', { name: 'Jon' } );
let user = make('user', { info });

// < Ember Data 3.16 this was the same object. Here the actual fragment was accepted by `store.push` as a valid property
console.log(info == user.info); // true

// Ember Data 3.16+ are no longer the same object. The passed-in fragment is used to generate the raw JSON data passed to `store.push`.
console.log(info == user.info); // false

While this is a breaking change I'm also working on rolling up a bunch of other changes into the v4 release (See #444)

@@ -129,7 +130,13 @@ export function containerSetup(application, serializerType) {
let store = application.lookup('service:store');

let adapterType = serializerType === '-json' ? '-rest' : serializerType;
let adapter = application.lookup('adapter:' + adapterType);
let adapter;
Copy link
Collaborator Author

@patocallaghan patocallaghan Oct 4, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These are test-only changes but it appears that doing the following out-of-the-box no longer works and the adapter is undefined.

application.lookup('adapter:-rest');

I need to register the RESTAdapter similar to the other adapters a few lines above to be able to retrieve it using lookup.

@patocallaghan patocallaghan force-pushed the patoc/ember-data-model-fragments-support branch from f589173 to 83aaf85 Compare October 4, 2021 15:23
@patocallaghan patocallaghan force-pushed the patoc/ember-data-model-fragments-support branch from 83aaf85 to c133c45 Compare October 4, 2021 15:41
@patocallaghan
Copy link
Collaborator Author

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 🟢.

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

Successfully merging this pull request may close these issues.

2 participants