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.

[lcmen]

@runspired could you provide more information how to do it? I'd like to try making this add-on compatible with Ember Data 3.28.

From my understanding fragments are basically computed properties on the model and they use _recordData to set / get fragment value. If we stored fragments as serialized values than how we can get appropriate full fragment instance when we call model.myFragmentProperty? How can we ensure that calling myFragmentProperty twice returns exactly same object?

Could you also explain what _recordData is, please? And how it differs from internalModel? It would help me to better understand the context. I tried to look api docs and source code I haven't found much info.

[lcmen]

@knownasilya I've just tested it on my project and #407 doesn't fix this issue - now I'm getting a following error:

Error: Assertion Failed: The `attr` method is not available on Model, a Snapshot was probably expected. Are you passing a Model instead of a Snapshot to your serializer?

@kevinkucharczyk can you check latest master on your project as well?

[knownasilya]

Can you create a failing test as a PR?

[lcmen]

@knownasilya there are already failing tests on existing test suite related to that error:

https://github.com/adopted-ember-addons/ember-data-model-fragments/runs/3613005831?check_suite_focus=true#step:7:641 https://github.com/adopted-ember-addons/ember-data-model-fragments/runs/3613005831?check_suite_focus=true#step:7:456

[adriguy]

@knownasilya I've just tested it on my project and #407 doesn't fix this issue - now I'm getting a following error:

Error: Assertion Failed: The `attr` method is not available on Model, a Snapshot was probably expected. Are you passing a Model instead of a Snapshot to your serializer?

@kevinkucharczyk can you check latest master on your project as well?

I encounter exactly the same issue than @lowski with the latest commit on master ☝️🤔

[AnDeVerin]

The same issue in our project.

[ngriselle]

As @lowski pointed out, the problem may be that before 3.28 the Snapshot for serialization is created by calling InternalModel.createSnapshot which is overridden in ext.js to recursively transform Fragments to Snapshots. In 3.28 the Snapshot for serialization is created by calling new Snapshot() directly instead so Fragments are no longer transformed to Snapshots.

I was able to dirty fix it by calling createSnapshot manually in the FragmentTransform.serialize method :

// transforms/fragment.js

serialize: function serializeFragment(snapshot) {
  if (!snapshot) {
    return null;
  }

  // Dirty fix for Ember Data 3.28
  // snapshot may be a Fragment instead of a Snapshot
  if (typeof snapshot._createSnapshot === 'function') {
    snapshot = snapshot._createSnapshot();
  }

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

  return serializer.serialize(snapshot);
},

// transforms/fragment-array.js

serialize: function serializeFragmentArray(snapshots) {
  if (!snapshots) {
    return null;
  }

  // Dirty fix for Ember Data 3.28
  // snapshots may be a Fragment instead of an Array<Snapshot>
  if (typeof snapshots._createSnapshot === 'function') {
    snapshots = snapshots._createSnapshot();
  }

  let store = this.store;

  return snapshots.map(snapshot => {
    let serializer = store.serializerFor(snapshot.modelName);
    return serializer.serialize(snapshot);
  });
}

There is probably a better way / place to do this.

[dwickern]

Here's a quick workaround,

import FragmentTransform from 'ember-data-model-fragments/transforms/fragment';
import FragmentArrayTransform from 'ember-data-model-fragments/transforms/fragment-array';

FragmentTransform.reopen({
  serialize(snapshot) {
    return this._super(snapshot?._createSnapshot?.() ?? snapshot);
  },
});

FragmentArrayTransform.reopen({
  serialize(snapshots) {
    return this._super(snapshots?._createSnapshot?.() ?? snapshots);
  },
});

[knownasilya]

I merged that fix, so master should work as well. Will try to research a bit more and see if there is a better solution.

[runspired]

The above fix is incomplete. Here's what did work in our app. Note I still am advocating for fixing the root cause (as currently we are hacking into internals instead of using record-data as designed), but this will help to unblock ember-data upgrades (there is still a separate issue I will open separately).

Fragment:

import Transform from "ember-data-model-fragments/transforms/fragment";

export default class Fragment extends Transform {
  serialize(snapshot) {
    if (!snapshot) {
      return null;
    }

    const { store } = this;
    const serializer = store.serializerFor(
      snapshot.modelName || snapshot.constructor.modelName
    );

    return serializer.serialize(
      snapshot.modelName ? snapshot : snapshot._internalModel.createSnapshot(),
      { includeId: true }
    );
  }
}

Fragment Array:

import { Snapshot } from "@ember-data/store/-private";
import Transform from "ember-data-model-fragments/transforms/fragment-array";

export default class FragmentArray extends Transform {
  serialize(snapshots) {
    if (!snapshots) {
      return null;
    }
    if (snapshots.length === 0) {
      return [];
    }
    const { store } = this;
    const arr = snapshots.toArray();
    const firstSnapshot = arr[0];
    const isNotYetSnapshot =
      !firstSnapshot.modelName && !(firstSnapshot instanceof Snapshot);
    const modelName = isNotYetSnapshot
      ? firstSnapshot.constructor.modelName
      : firstSnapshot.modelName;
    const serializer = store.serializerFor(modelName);

    return arr.map((s) =>
      serializer.serialize(
        s && isNotYetSnapshot ? s._internalModel.createSnapshot() : s,
        { includeId: true }
      )
    );
  }
}

[dwickern]

There's also an issue with property change notifications for hasDirtyAttributes on fragment arrays, which accounts for some test failures on the master branch.

I did attempt doing things the "right way" with record-data. I'll open a PR.

[runspired]

There's also an issue with property change notifications for hasDirtyAttributes on fragment arrays, which accounts for some test failures on the master branch.

I did attempt doing things the "right way" with record-data. I'll open a PR.

That's the other issue. It's due ember-data removing the state machine.

[enspandi]

The above fix is incomplete. Here's what did work in our app. Note I still am advocating for fixing the root cause (as currently we are hacking into internals instead of using record-data as designed), but this will help to unblock ember-data upgrades (there is still a separate issue I will open separately).

Fragment:

import Transform from "ember-data-model-fragments/transforms/fragment";

export default class Fragment extends Transform {
  serialize(snapshot) {
    if (!snapshot) {
      return null;
    }

    const { store } = this;
    const serializer = store.serializerFor(
      snapshot.modelName || snapshot.constructor.modelName
    );

    return serializer.serialize(
      snapshot.modelName ? snapshot : snapshot._internalModel.createSnapshot(),
      { includeId: true }
    );
  }
}

Fragment Array:

import { Snapshot } from "@ember-data/store/-private";
import Transform from "ember-data-model-fragments/transforms/fragment-array";

export default class FragmentArray extends Transform {
  serialize(snapshots) {
    if (!snapshots) {
      return null;
    }
    if (snapshots.length === 0) {
      return [];
    }
    const { store } = this;
    const arr = snapshots.toArray();
    const firstSnapshot = arr[0];
    const isNotYetSnapshot =
      !firstSnapshot.modelName && !(firstSnapshot instanceof Snapshot);
    const modelName = isNotYetSnapshot
      ? firstSnapshot.constructor.modelName
      : firstSnapshot.modelName;
    const serializer = store.serializerFor(modelName);

    return arr.map((s) =>
      serializer.serialize(
        s && isNotYetSnapshot ? s._internalModel.createSnapshot() : s,
        { includeId: true }
      )
    );
  }
}

Thx @runspired ! The patch worked very well for our 3.28 upgrade. We only had to adapt it a bit to work with polymorphic fragmentArray relationship (essentially instead of using firstSnapshot we run the same logic for each snapshot in arr.map)

[VincentMolinie]

Could you try with the latest release @kevinkucharczyk ?