Model fragments broken against Ember Data 3.28

[kevinkucharczyk]

I was testing Ember Data Model Fragments against the latest Ember Data - version 3.28 - and unfortunately it looks like they're not compatible. Here's what happens when I try to save a model that contains a fragment:

index.js:178 Uncaught Error: Assertion Failed: You need to pass a model name to the store's serializerFor method
    at assert (index.js:178)
    at Store.serializerFor (ext.js:230)
    at Store.superWrapper [as serializerFor] (index.js:442)
    at Class.serializeFragment [as serialize] (fragment.js:40)
    at ApplicationSerializer.serializeAttribute (json.js:1105)
    at json.js:1035
    at -private.js:1611
    at Array.forEach (<anonymous>)
    at Snapshot.eachAttribute (-private.js:1610)
    at ApplicationSerializer.serialize (json.js:1034)
assert @ index.js:178
serializerFor @ ext.js:230
superWrapper @ index.js:442
serializeFragment @ fragment.js:40
serializeAttribute @ json.js:1105
(anonymous) @ json.js:1035
(anonymous) @ -private.js:1611
eachAttribute @ -private.js:1610
serialize @ json.js:1034
serialize @ rest.js:584
superWrapper @ index.js:442
serializeIntoHash @ rest.js:612
serializeIntoHash @ -private.js:634
createRecord @ rest.js:694
(anonymous) @ -private.js:1867
invokeCallback @ rsvp.js:493
(anonymous) @ rsvp.js:558
(anonymous) @ rsvp.js:19
invoke @ backburner.js:338
flush @ backburner.js:229
flush @ backburner.js:426
_end @ backburner.js:958
end @ backburner.js:708
_run @ backburner.js:1013
run @ backburner.js:752
run @ index.js:116
callback @ application.js:434

This particular problem seems to happen at

ember-data-model-fragments/addon/transforms/fragment.js

Line 38 in b1a661b

let serializer = store.serializerFor(snapshot.modelName);

The serialize method is expecting a Snapshot, but actually receives the model instance of the fragment (I'm not sure why) - modelName is empty there.

Here's a minimal reproduction on a fresh Ember app:
ember-quickstart.zip

[knownasilya]

Which version of this addon did you use?

[kevinkucharczyk]

@knownasilya I used v5.0.0-beta.2

[knownasilya]

Any chance you can make a small reproduction app?

[kevinkucharczyk]

@knownasilya I've already included a zipped reproduction app in my first comment, at the very end is a link:

[knownasilya]

🤦‍♂️

[lcmen]

I can confirm this. I just bumped my app to 3.28.0 and was bitten by the same error.

[knownasilya]

Give #407 a try.
Update package.json to

"ember-data-model-fragments": "knownasilya/ember-data-model-fragments#e22f3b80285f"

And npm/yarn install.

[kevinkucharczyk]

@knownasilya that fix is not quite enough, unfortunately. If you have a fragment array, that'll fail too under 3.28 (it requires a similar fix in the fragment array serializer).

Either way, I don't think the proposed fix in the PR is the right way to go. What strikes me as weird is the fact that what's passed in is no longer a snapshot. Is that expected behavior? What exactly changed in Ember/Ember Data that caused this? I feel like patching the transforms like that may still cause issues elsewhere down the line if we don't know what's happening under the hood.

[lcmen]

@knownasilya @kevinkucharczyk I believe found the culprit: CoreStore in 3.27.1 doesn't have REQUEST_SERVICE flag enabled (in 3.28.1 it's already on) so scheduleSave relies on pendingSave queue (which uses internalModel.createSnapshot(options) to create model's snapshots):

let snapshot = internalModel.createSnapshot(options);
this._pendingSave.push({
  snapshot: snapshot,
  resolver: resolver,
});

emberBackburner.scheduleOnce('actions', this, this.flushPendingSave);

In 3.28.1 scheduleSave goes through fetchManager (via if (REQUEST_SERVICE) { ... } conditional) and it uses Snapshot constructor to build model's snapshot:

let snapshot = new Snapshot(options, identifier, this._store);

For some reason, fragments are not correctly converted to snapshots in this case. I don't know if it's bug or desired behavior.

I'm not an Ember expert by any means so I'm not sure what's the most appropriate solution but in 3.28.1 serializer.serialize(snapshot) operates on Fragment instead of Snapshot. I don't know what consequences it might have 🤷 .

[knownasilya]

@runspired any advice?

[runspired]

@lowski internalModel.createSnapshot uses the same way to create the Snapshot

https://github.com/emberjs/data/blob/584c14b5181513756750e65b73338cde7244d1a1/packages/store/addon/-private/system/model/internal-model.ts#L964

The underlying issue here is more likely related to the activation of CUSTOM_MODEL_CLASS features, this library still hasn't fully converted to the record-data approach to fragments and likely this leads this code and this code in Snapshot to behave differently. Probably the FragmentRecordData is storying the fragment as the value within the record-data instead of the serialized value.

[lcmen]

@knownasilya I've looked into this again and it looks this addon overrides internalModel.createSnapshot in addon/ext.js file. By doing so, it can traverse all attributes and call createSnapshot recursively. When you use new Snapshot() in fetchManager.scheduleSave, this no longer works.

@runspired what is the best way to modify scheduleSave in so it uses internalModel.createSnapshot? Could we for example replace: new Snapshot(options, identifier, this._store) with _store._internalModelForResource(identifier).createSnapshot(options)? Or maybe it's a wrong direction and we should try to override get _attributes() in Snapshot class (if so, how can we do it)?

[runspired]

The right way to do this is to store the serialized form in the fragment RecordData, no private apis would be necessary then.