Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

spike for prop notification #586

Open
wants to merge 17 commits into
base: master
Choose a base branch
from
55 changes: 46 additions & 9 deletions addon/index.js
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
import { assert } from '@ember/debug';
import { dependentKeyCompat } from '@ember/object/compat';
import { BufferedChangeset } from 'validated-changeset';
import { BufferedChangeset, Change, keyInObject } from 'validated-changeset';
import ArrayProxy from '@ember/array/proxy';
import ObjectProxy from '@ember/object/proxy';
import { notifyPropertyChange } from '@ember/object';
Expand Down Expand Up @@ -32,6 +32,33 @@ function maybeUnwrapProxy(o) {
return isProxy(o) ? maybeUnwrapProxy(safeGet(o, 'content')) : o;
}

function tryContent(ctx) {
return ctx._content ? ctx._content : ctx.content ? ctx.content : ctx;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is _content meant to capture from the Changeset and content from an ObjectProxyNode?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, that's correct.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sry for stringing this along but I do want to get a solution in. I'm learning from your work to understand the issue better.

So is it best to say, not only does notifyPropertyChange not work for nested keys (this is obvious to me), but also we are notifying on the wrong object? Meaning notifyPropertyChange(this, key) leads to the wrong/no expected behaviour as well? I wouldn't have through this would have work for an ObjectProxyNode (non Ember object, etc)

}

function deepNotifyPropertyChange(changeset, path) {
let paths = path.split('.');
let maybeDynamicPathToNotify = null,
lastPath = paths.pop(),
current = changeset,
i;

//If the path doesn't exists previously exist inside the CONTENT, this is a dynamic set.
let existsInContent = safeGet(changeset[CONTENT], path) !== undefined;

for (i = 0; i < paths.length; ++i) {
const curr = current[paths[i]];
if (existsInContent && curr && curr.content === curr.content) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

curr will fail for falsey values, correct?

Do we want to use getPrototypeOf and walk the chain checking for getOwnPropertyDescriptor? Or perhaps we just directly check hasOwnProperty on the object.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I think that makes sense

current = current[paths[i]];
} else {
maybeDynamicPathToNotify = paths[i];
break;
}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If I'm understanding this correctly, we want to traverse down the object to the leaf key, calling notify property change as we go? Or do we want to reduce property notifications (as indicated in the issue) and only want to notify at the leaf key in paths?

Two questions.

  1. Does the if else blocks need to be switched? If != undefined, we want to notifyProperty change at that level?
  2. Does it make more sense to start at the leave and traverse up? Or rather, start at the last key in paths and iterate backwards?

Copy link
Contributor Author

@betocantu93 betocantu93 Apr 15, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Rewrote my last comment as my understanding changed.

The idea is to traverse down to find the deepest defined ancestor.

example 1.
Take: key as foo.bar.baz and data as { foo: { bar: {} } }

The output should: notifyPropertyChange(data.foo.bar, 'baz')

example 2.
Take: key as foo.bar.baz and data as { }, everything undefined

The output should: notifyPropertyChange(data, 'foo')

example 3.
Take: key as foo.bar.baz and data as { foo: {} }, so bar undeefined

The output should: notifyPropertyChange(data.foo, 'bar')

I think this could be optimized like this:

for (i = 0; i < paths.length; ++i) {
    if (current[paths[i]] != undefined) { // we can still go deep
      current = current[paths[i]];
    } else {
      //we just found that its a dynamic set, so we notify here and stop, no point in going further. 
      notifyPropertyChange(tryContent(current), paths[i]);
      return;
    }
}
//We didn't stop, we haven't notified, and we went the deepest as possible, so we notify there.
 notifyPropertyChange(tryContent(current), lastPath);

I think its ok to start from the top, because we can early notify and return

}
const pathToNotify = maybeDynamicPathToNotify ? maybeDynamicPathToNotify : lastPath;
notifyPropertyChange(tryContent(current), pathToNotify);
}

export class EmberChangeset extends BufferedChangeset {
@tracked _changes;
@tracked _errors;
Expand Down Expand Up @@ -110,7 +137,7 @@ export class EmberChangeset extends BufferedChangeset {
addError(key, error) {
super.addError(key, error);

notifyPropertyChange(this, key);
deepNotifyPropertyChange(this, key);
// Return passed-in `error`.
return error;
}
Expand All @@ -123,7 +150,7 @@ export class EmberChangeset extends BufferedChangeset {
pushErrors(key, ...newErrors) {
const { value, validation } = super.pushErrors(key, ...newErrors);

notifyPropertyChange(this, key);
deepNotifyPropertyChange(this, key);

return { value, validation };
}
Expand All @@ -133,9 +160,21 @@ export class EmberChangeset extends BufferedChangeset {
* Returns value or error
*/
_setProperty({ key, value, oldValue }) {
super._setProperty({ key, value, oldValue });
let changes = this[CHANGES];

notifyPropertyChange(this, key);
//We always set
this.setDeep(changes, key, new Change(value), {
safeSet: this.safeSet,
});

//If the newValue is equals to the wrapped value, delete key from changes
if (oldValue === value && keyInObject(changes, key)) {
this._deleteKey(CHANGES, key);
}

//Notitify deep
deepNotifyPropertyChange(this, key);
notifyPropertyChange(this, 'changes');
}

/**
Expand All @@ -149,7 +188,7 @@ export class EmberChangeset extends BufferedChangeset {
_notifyVirtualProperties(keys) {
keys = super._notifyVirtualProperties(keys);

(keys || []).forEach((key) => notifyPropertyChange(this, key));
(keys || []).forEach((key) => deepNotifyPropertyChange(this, key));

return;
}
Expand All @@ -159,9 +198,7 @@ export class EmberChangeset extends BufferedChangeset {
*/
_deleteKey(objName, key = '') {
const result = super._deleteKey(objName, key);

notifyPropertyChange(this, key);

deepNotifyPropertyChange(this, key);
return result;
}

Expand Down
21 changes: 19 additions & 2 deletions tests/unit/changeset-test.js
Original file line number Diff line number Diff line change
Expand Up @@ -1780,15 +1780,32 @@ module('Unit | Utility | changeset', function (hooks) {
test('observing #rollback values', async function (assert) {
let res;
let changeset = Changeset(dummyModel, dummyValidator);
changeset.addObserver('name', function () {
res = this.name;
//To consistently observe for changes (even for deep keys),
//we must adhere to a never changing value
//so the changeset.data (_content) is the silver bullet for now
//ember-changeset will keep notifying on it
//one downside is that you won't get the changeset as `this` inside the callback.
changeset.data.addObserver('name', function () {
res = changeset.get('name');
});
assert.equal(undefined, changeset.get('name'), 'initial value');
changeset.set('name', 'Jack');
assert.equal('Jack', res, 'observer fired when setting value');
changeset.rollback();
assert.equal(undefined, res, 'observer fired with the value name was rollback to');
});
test('observing deep keys on .data works', async function (assert) {
let res;
let changeset = Changeset(dummyModel, dummyValidator);
changeset.data.addObserver('some.really.nested.path', function () {
res = changeset.get('some.really.nested.path');
});
assert.equal(undefined, changeset.get('some.really.nested.path'), 'initial value');
changeset.set('some.really.nested.path', 'Jack');
assert.equal('Jack', res, 'observer fired when setting value some.really.nested.path');
changeset.rollback();
assert.equal(undefined, res, 'observer fired with the value some.really.nested.path was rollback to');
betocantu93 marked this conversation as resolved.
Show resolved Hide resolved
});

test('can update nested keys after rollback changes.', async function (assert) {
let expectedResult = {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

#585

Do we have a test for this issue so we can close it out when merged?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ping! Lmk what you think about adding this test. Lots of great work in this PR.

Copy link
Contributor Author

@betocantu93 betocantu93 Apr 23, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sorry for the delay, I ran that repo, I think it has a bug on the example itself, it uses

@value={{not (changeset-get this.changesetObj contextKey)}}
@onToggle={{action (changeset-set this.changesetObj contextKey) (not (changeset-get this.changesetObj contextKey))}}

I think that action helper is actually(?) mutating the CONTENT, somehow(?) and thus it actually have changes

but I just ran that branch with a few changes to the example and its working!

@value={{changeset-get this.changesetObj contextKey}}
@onToggle={{changeset-set
          this.changesetObj
          contextKey
          (not (changeset-get this.changesetObj contextKey))
        }}

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And so, I think... no further test need to be added? 🤔 I mean there are a few tests already checking for changes emptied after setting back the initial wrapped value? gonan double check

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm I ran this again and the bug still exists, but I'm unable to track why it seems somehow sets are applied to the this[CONTENT] without executing the changeset using changeset-set.

Expand Down