adopted-ember-addons / ember-changeset

Ember.js flavored changesets, inspired by Ecto
http://bit.ly/ember-changeset-demo
MIT License
431 stars 141 forks source link

isDirty does not trigger dependent property changes #560

Closed williamhaley closed 2 years ago

williamhaley commented 3 years ago

Version

ember-changeset 3.10.1

Test Case

  test('changeset isDirty property notifies dependent computed properties', async function (assert) {
    class Model extends EmberObject {
      // single
      @computed
      get changeset() {
        return Changeset(dummyModel, dummyValidator);
      }

      //
      // THIS IS MY MODIFICATION. I added @computed('changeset.isDirty')
      // THIS WAS A PURE get BEFORE.
      //
      @computed('changeset.isDirty')
      get isChangesetDirty() {
        return this.changeset.isDirty;
      }

      // double
      @computed
      get changesets() {
        let arr = [Changeset(dummyModel, dummyValidator), Changeset(dummyModel, dummyValidator)];
        return arr;
      }

      get hasDirtyChangesets() {
        return this.dirtyChangesets.length > 0;
      }

      get dirtyChangesets() {
        return this.changesets.filter((c) => c.isDirty);
      }
    }

    let model = Model.create();

    assert.notOk(get(model, 'hasDirtyChangesets'), 'no dirty changesets');
    assert.equal(model.dirtyChangesets.length, 0, 'has 0 dirty changesets');
    assert.equal(get(model, 'isChangesetDirty'), false, 'changeset not dirty');

    let changesets = get(model, 'changesets');

    changesets[0].set('name', 'new name');
    assert.equal(model.dirtyChangesets.length, 1, 'has one dirty changesets');
    assert.ok(get(model, 'hasDirtyChangesets'), 'has dirty changesets');

    let changeset = get(model, 'changeset');
    set(changeset, 'name', 'other new name');
    assert.equal(get(model, 'isChangesetDirty'), true, 'changeset is dirty');
  });

Steps to reproduce

See test case above with my all-caps comment. If you add @computed('changeset.isDirty') from the isChangesetDirty getter it stops tracking isDirty updates properly. It only works if it's a pure getter.

Expected Behavior

I'd expect that there's nothing wrong with having a computed prop with dependencies calculated from isDirty on a changeset.

Actual Behavior

See steps to reproduce above. The test fails, and in my real life app, the computed property that I use never updates because the changeset.isDirty never seems to trigger it.


I used to do something like this with Ember Octane on ember-changeset 2.x

@or('changeset1.isDirty', 'changese2.isDirty', 'changeset3.isDirty')

I kept my Ember Octane at the same version but bumped up to ember-changeset 3.x. Once I did, this @or helper above stopped working. The data is stale and doesn't update when one of the dependent properties updates.

Now I have to do this.

get isDirty() {
    return this.changeset1.isDirty || this.changeset2.isDirty || this.changeset3.isDirty;
}

Not a huge deal, but a subtle breaking change for me between ember-changeset 2.x and 3.x that took a while to figure out.

mroetheli commented 3 years ago

I have a similar issue. I used this working piece of code (I've simplified it for this report):

errors: computed('changeset.errors', function() {
  let errors = this.changeset.get('errors');
  if (errors.any(error => error.key == 'key'
    return true;
  } else {
    return false;
  }
}),

No changes in changeset or changeset.errors are no longer registered. Do I have to refer differently to changeset or is there a new solution? I use ember-changeset 3.8.0. I also does not work with the newest version.

machty commented 2 years ago

I'm running into this issue; is the official advice/solution that any CPs that depend on changeset.isDirty need to be rewritten as native getters?

snewcomer commented 2 years ago

Ya I believe so.

I was just playing with the example above and without computeds it works. With, it doesn't. I did add dependentKeyCompat blindly, but it seems to not have resolved it either.

https://github.com/validated-changeset/validated-changeset/issues/164