Improved Ember Data and Ember Changeset support for relationships

[snewcomer]

Main problem

Create a changeset with a model, set a belongsTo relationship, change a belongsTo, and lastly revert back to original - the changeset will be dirty

Support for ember-data relationship objects has been fraught with issues. Precisely and often with async (default) relationships in Ember Data. This is due to them being behind a Proxy, thus a === will fail with the same relationship.

This issue is to track (and keep me honest) in either implementing better support for ember data relationships or improved documentation.

One idea that is floating around is to use @embroider/macros to see if e-d is installed and then we can importSync the model and do an instanceof Model check. I'll be exploring this space in the next few weeks.

[snewcomer]

Also here is a working implementation using the Reference API and extending ember-changeset. (thanks Oliver!)

import { EmberChangeset } from 'ember-changeset';
import { TrackedArray } from 'tracked-built-ins';
import isEqual from 'lodash/isEqual';

/**
 * Ember Changeset copies Ember Data relationships,
 * which are a proxy to the actual related value.
 *
 * `hasMany` relationships are always treated as changes,
 * so manipulations to the array do not affect the underlying relationship
 * until the changeset is executed.
 */
export default class RawRelatedModelsChangeset extends EmberChangeset {
  // keys for the hasMany relationships we're tracking
  hasManyProperties = [];

  /**
   * Returns the unwrapped related model for `belongsTo` relationships,
   * or a TrackedArray of models (for `hasMany`).
   *
   * @inheritdoc
   * @override
   */
  get(key) {
    if (this.data[key] && !this._changes[key]) {
      switch (this.data.relationshipFor?.(key)?.meta?.kind) {
        case 'belongsTo':
          return this.data.belongsTo(key).value();
        case 'hasMany':
          this.hasManyProperties.push(key);
          this._changes[key] = new TrackedArray(this.data.get(key).toArray());
          break; // the change will be returned by `super.get`
        default:
          break;
      }
    }

    return super.get(key);
  }

  /**
   * `safeGet` returns the original value for comparison,
   * but not when initially fetched (which is done in `get`)
   *
   * @inheritdoc
   * @override
   */
  safeGet(obj, key) {
    if (obj.relationshipFor?.(key)?.meta?.kind == 'belongsTo') {
      return obj.belongsTo(key).value();
    }
    return super.safeGet(obj, key);
  }

  /**
   * Because `hasMany` are TrackedArrays,
   * this will recompute when they change
   * (assuming they are used in a tracking context).
   *
   * Also contains an override to correctly compare Date objects.
   *
   * @inheritdoc
   * @override
   */
  get isPristine() {
    let pristine = super.isPristine;

    if (!pristine) {
      const allDates = this.changes.filter(({ value }) => value instanceof Date);

      // if the only changes are dates,
      // and those are all the same,
      // override pristine state
      if (this.changes.length == allDates.length) {
        pristine = allDates.every(({ key, value }) => {
          return value.getTime() == this.data.get(key).getTime();
        });
      }
    }

    // check the arrays and see if any differ
    if (pristine) {
      const arraysDiffer = this.hasManyProperties.find(prop => {
        return this._changes[prop]
          && !isEqual(this._changes[prop], this.data.get(prop).toArray());
      });

      if (arraysDiffer) {
        return false;
      }
    }

    return pristine;
  }
}

[sandstrom]

Just wanted to say that I totally encourage thinking about this! It's not a simple problem, but solve it would be very useful!🏅

1. Also, it's worth considering whether this should be tightly coupled with Ember data, or something more generic that could work with multiple data libraries?
   I'd lean towards something specific for Ember Data (I think people tend to overestimate generic solutions inside of frameworks, since people "within" a framework are mostly going to use the paved path anyway). However, both approaches would obviously work!

2. One alternative could also consider going at it from the Ember Data side, i.e. what changes could we make in Ember Data to make this easier. An obvious one would be snapshots/restore. If they existed, would that make things easier on the Ember Changeset side?

3. Also, I've copied some thoughts from a related thread here:

One idea could be to extend the basic principle of the changeset to relationships.

It's already a proxy at the key/value surface of an object, basically get/set is proxied and buffered. But this "leaks" is if myChangeset.children return an array (or an array-like relationship object from Ember Data).

One approach would be to "tell" the changeset, when initiated, about the keys that return relationships, for example new Changeset(object, { listKeys: ['children'] }). For these keys, instead of proxying down to the underlying model the changeset could create a ChangeSetList (on-the-fly), associate this with the changeset and wrap it around the relationship [array or similar] and return it as the response.

myChangeset.children would then return a ChangeSetList with methods such as add and remove, plus support iteration, e.g. via toArray or Symbol.iterator. Adding something to the changeset-list, via add, would then buffer/proxy just like get and set already does on a changeset. This would allow rollback/save of added/removed relationship items.

Happy to clarify my thinking about this and explain in more detail!

Source: #551 (comment)

[snewcomer]

@sandstrom Appreciate the comments!

So in considering both approaches...

1. take a list of "relationship" keys and assume we are talking about an e-d model? If so, then we wrap with a construct that knows how to work with e-d models specifically?
2. Use the power of introspection and determine if we are dealing with a Model instance and then act accordingly.

Perhaps they may both work in concert but do I have the overall train of thought right? Do you have any specific concerns with, say the safeGet in the method above?

[sandstrom]

My comments are a bit confusing, because I'm throwing several ideas around. But I guess I have two main ideas (1 and 2) when thinking of the overall problem with tracking changes to graphs of Ember Data models.

1. Make the changes in Ember Data, e.g. add support for snapshots (that kept track of relationship changes). 95% of the work would occur in Ember Data, and Ember changeset could simply work with those snapshots. Does sort of side-step Ember Changeset though, but I guess it still makes sense to put this idea forward, even though there are also other approaches.

2. Add generic functionality to Ember Changeset for tracking "list" values. Right now, Ember Changeset can handle attributes (value is a primitive) and belongs to (value is a singular thing), but has trouble with "list like" things, like an array or a hasMany relationship.

   One solution could be to "tell" the changeset, when initiated, about the keys that return relationships, for example new let myChangeset = Changeset(object, { listKeys: ['children'] }). For these keys, instead of proxying down to the underlying model, the changeset would create a ChangeSetList (on-the-fly), associate this with the changeset and wrap it around the relationship [array or similar] and return it as the response.

   Just as a changeset today keep track of changes, it would then also keep track of associated ChangeSetList instances, and whether those contained any changes (add/remove).

   myChangeset.children would then return a ChangeSetList instance, with methods such as add and remove, plus support for iteration (via toArray or Symbol.iterator). Adding something to the changeset-list, via add, would then buffer/proxy just like get and set already does on a changeset. This would allow rollback/save of added/removed relationship items.

   I've copied a small example class below to illustrate further.

Your suggestion above (#599 (comment)) would probably also work well, but is somewhat different from the ideas (1) and (2).

So I'm not trying to argue against it. I merely wanted to present two alternative ways of approaching this.

But you are in a much better position to determine a good way forward than I am 😄

// these classes are far from complete, but hopefully help illustrate the idea

class ChangeSetList {
  constructor(changeSet, items) {
    this.items = items;
    this.changeSet = changeSet;
    this.addedItems = new Set();
    this.removedItems = new Set();
  }

  add(item) {
    this.addedItems.add(item);
  }

  remove(item) {
    if (this.addedItems.has(item)) {
      // don't track, not part of original items
      this.addedItems.delete(item);
    } else {
      this.removedItems.add(item);
    }
  }

  // synthetic list based on non-deleted and added items
  toArray() {
    this.items.filter((itm) => {
      this.removedItems.has(itm);
    }).concat(this.addedItems);
  }

  hasChanges() {
    this.addedItems.size > 0 || this.removedItems.size > 0;
  }

}

class ChangeSet {
  constructor(source, options = {}) {
    this.source = source;
    this.listKeys = options.listKeys || [];
    this.instantiatedLists = new Map();
  }

  get(key) {
    if (this.listKeys.includes(key)) {
      if (this.instantiatedLists.has(key)) {
        return this.instantiatedLists.get(key);
      } else {
        let list = new ChangeSetList(this, )
        this.instantiatedLists.set(key, list);
        return list;
      }
    } else {
      // run whatever logic ChangeSet has today
    }
  }

  set(key, value) {
    if (this.listKeys.includes(key)) {
      throw new Error(`Cannot set a list key, use myChangeSet.get('${key}').add(itm), or .remove(itm) instead`);
    } else {
      // run whatever logic ChangeSet has today
    }
  }
}

[sandstrom]

We could check in with @richbrown @oliverlangan @Gorzas and see if they have any thoughts.

[snewcomer]

Thx for the comments! Data changes are probably a no go. The complexity around tracking changes is quite large. I believe there is a lot of simplicity coming. But not there yet.

RE: your ideas - I think this is probably good for a different conversation. This issue is specific to detecting and using the existing Ember Data APIs to work with relationships - even singular belongsTo relationships. Your ideas are probably nice to think about/tackle once we can properly detect a Model instance.

[sandstrom]

What I described in point 2 shouldn't need any awareness of Ember Data, i.e. it shouldn't matter if the object is a POJO with an array value for one of the keys, or if the object is an Ember Data model.

[Gorzas]

I don't have enough experience with Ember Changeset to have a clear idea so I don't know if I really can help. Anyway, if you need somebody to test the changes, I'll be glad to help you with it 😄 .

[pernambucano]

Hi, @snewcomer, thanks a lot for all the (great) work on this lib.
As I understand we don't have a de-facto way to handle hasMany relationships right now, is that right? I saw that something was implemented towards this goal (adopted-ember-addons/validated-changeset#104), but it got me confused about how to use that in a scenario where we have a form, and inside the form a hasMany relationship (think about creating a form for a User Model, with a name and many contacts, where each contact is an instance of a Contact Model). In this scenario, I usually create a new changeset for each contact instance and then add/handle the save manually as well. Is there a better direction to go here? Is there any example with hbs and js/ts code on this?

Again, thanks a lot!

[sandstrom]

@pernambucano (and anyone else who's interested in this), consider sponsoring @snewcomer via Github sponsors, so he can spend more time on this library (ask your employer to pay). We sponsor him with $100 every month.

[tschoartschi]

@snewcomer thanks for this awesome fix 🥳 we are currently upgrading to Ember-Data 5.x and we had huge troubles with ember-changeset. The idea of @snewcomer saved us and now we can continue our upgrade!