adopted-ember-addons / validated-changeset

Buffering changes to form data
MIT License
36 stars 27 forks source link

Snapshot changes is wrong when object has proxy attributes #102

Open urbany opened 3 years ago

urbany commented 3 years ago

Hi, changeset of ED Models with async belongsTo relationships show incorrect changes on snapshot: Screenshot 2021-02-03 at 16 15 37

change and changes are correct, but snapshot().changes shows all my async belongsTo as changed attributes incorrectly.

I think this is because of this unrwrap https://github.com/validated-changeset/validated-changeset/blob/da68651a41028ab827872cf10960920779c2a09f/src/index.ts#L975

Which adds all the unwrapped relationships to this[CHANGES]. When i get change and changes these incorrect changes are filtered out, but not on snapshot.

Does my explanation make sense? Thanks in advance!

EDIT: If anyone else comes here looking for a solution I'm currently creating snapshots like this until it is fixed

function createSnapshot(cs) {
  const changes = {};
  cs.changes.forEach(({ key, value }) => { changes[key] = value; });
  return {
    changes,
    errors: { ...cs.error },
  };
}
urbany commented 3 years ago

Screenshot 2021-02-03 at 17 40 44

Actually after some more tests changeset.change is also broken, changeset.changesis the only one showing correct changes. changeset.change becomes wrong only if there is a real change in the changeset (in the example above it is the attr state)

snewcomer commented 3 years ago

@urbany Thx for the issue! Do you have an example test we could add here? It would be good to know how you get to this state!

DLiblik commented 3 years ago

We also see this inconsistency. The fastest way to get there is to start with a POJO model that has a sub-key with an attribute set and then create a changeset off of it. Then call set on the changeset to add another key to the same sub-key. Calling snapshot will render an output in changes that includes the sub-key, but it is an empty object. My understanding of changes is that it lists paths on a top-level object, not nested objects - i.e. that a change to a path on the changeset of student.firstName should show on changes as changes['student.firstName']: 'Bob' not changes.student: { firstName: 'Bob' } which is what I would expect in change (not changes).

... you can also hit it quickly by simply having a change on a subkey, then snapshotting and restoring the changeset - that should effectively be a no-op, but actually it clears out the changes on all the nested paths.

DLiblik commented 3 years ago

...I should add that we just upgraded to v3 from v2 of ember-changeset and hit this there as v3 picks up this version. This has had some catastrophic consequences as we did not initially discover it (our bad) in our testing and now we can't go back due to other dependencies. This is not an obvious condition to detect - it comes out as odd behaviour in user-side support complaints. I say all of this because once resolved, you may wish to mark the tags with this issue as deprecated to help others move past it.

snewcomer commented 3 years ago

If someone has a failing test they would like to contribute that would be phenomenal! I can continue the work if that is something you didn't want to take on.

DLiblik commented 3 years ago

Just run this - I would expect by contract of the changeset that the console entries would match.

const cs = new Changeset({ person: { firstName: 'Shep' } });
cs.set('person.firstName', 'Moe');
console.log(cs.change);
cs.restore(cs.snapshot());
console.log(cs.change);