-
-
Notifications
You must be signed in to change notification settings - Fork 141
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
base: master
Are you sure you want to change the base?
Changes from 13 commits
b38d79c
a794db2
b780c13
4d1469a
907ee21
b232cd9
65b528b
a186905
68c426e
4ad58ce
a93ddeb
a345077
17380df
f680506
d59a4ec
218ff11
b5f30fe
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
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'; | ||
|
@@ -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; | ||
} | ||
|
||
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) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Do we want to use There was a problem hiding this comment. Choose a reason for hiding this commentThe 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; | ||
} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 Two questions.
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 example 1. The output should: example 2. The output should: example 3. The output should: 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; | ||
|
@@ -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; | ||
} | ||
|
@@ -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 }; | ||
} | ||
|
@@ -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'); | ||
} | ||
|
||
/** | ||
|
@@ -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; | ||
} | ||
|
@@ -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; | ||
} | ||
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -75,12 +75,12 @@ | |
"release-it-lerna-changelog": "^1.0.3" | ||
}, | ||
"engines": { | ||
"node": "10.* || >= 12.*" | ||
"node": ">= 12.*" | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This will be a major version bump I believe. What was the specific error? I thought v10 had another 6 days until EOL. |
||
}, | ||
"ember-addon": { | ||
"configPath": "tests/dummy/config" | ||
}, | ||
"volta": { | ||
"node": "10.22.0" | ||
"node": "12.22.1" | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -1780,15 +1780,33 @@ 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, changeset.get('some.really.nested.path'), 'initial value is back to undefined'); | ||
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 = { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Do we have a test for this issue so we can close it out when merged? There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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))
}}
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 |
||
|
There was a problem hiding this comment.
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 andcontent
from an ObjectProxyNode?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, that's correct.
There was a problem hiding this comment.
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)