-
-
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
Tracked mirror approach for prop notifications #591
base: master
Are you sure you want to change the base?
Changes from all commits
b38d79c
a794db2
b780c13
4d1469a
907ee21
b232cd9
65b528b
a186905
68c426e
4ad58ce
a93ddeb
a345077
17380df
f680506
d59a4ec
218ff11
4c33927
7e06536
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,17 +1,19 @@ | ||
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'; | ||
import mergeDeep from './utils/merge-deep'; | ||
import isObject from './utils/is-object'; | ||
import { tracked } from '@glimmer/tracking'; | ||
import { tracked } from 'tracked-built-ins'; | ||
import { get as safeGet, set as safeSet } from '@ember/object'; | ||
import { addObserver, removeObserver } from '@ember/object/observers'; | ||
|
||
const CHANGES = '_changes'; | ||
const PREVIOUS_CONTENT = '_previousContent'; | ||
const CONTENT = '_content'; | ||
const VIRTUAL_MIRROR = '_virtualMirror'; | ||
const defaultValidatorFn = () => true; | ||
|
||
export function buildOldValues(content, changes, getDeep) { | ||
|
@@ -32,10 +34,80 @@ function maybeUnwrapProxy(o) { | |
return isProxy(o) ? maybeUnwrapProxy(safeGet(o, 'content')) : o; | ||
} | ||
|
||
function deepNotifyPropertyChange(changeset, path) { | ||
let paths = path.split('.'); | ||
let maybeDynamicPathToNotify = null, | ||
lastPath = paths.pop(), | ||
current = changeset, | ||
i; | ||
|
||
for (i = 0; i < paths.length; ++i) { | ||
if (Object.prototype.hasOwnProperty.call(current, paths[i])) { | ||
current = safeGet(current, paths[i]); | ||
} else { | ||
maybeDynamicPathToNotify = paths[i]; | ||
break; | ||
} | ||
} | ||
const pathToNotify = maybeDynamicPathToNotify ? maybeDynamicPathToNotify : lastPath; | ||
notifyPropertyChange(current, pathToNotify); | ||
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. I wonder if we should make 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. Or am I missing a crucial need for 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. 🤔🤔🤔 maybe it's no longer needed, you're right, gonna have a look, one thing I still need to look for is reactivity around arrays, i.e. computeds which use |
||
} | ||
|
||
function deepHasOwnProperty(obj, path) { | ||
if (!path) { | ||
return false; | ||
} | ||
|
||
let paths = path.split('.'), | ||
current = obj, | ||
i; | ||
|
||
for (i = 0; i < paths.length; ++i) { | ||
if (!current || !Object.prototype.hasOwnProperty.call(current, paths[i])) { | ||
return false; | ||
} else { | ||
current = current[paths[i]]; | ||
} | ||
} | ||
|
||
return true; | ||
} | ||
|
||
function deepAddObserver(obj, path, callback) { | ||
let paths = path.split('.'); | ||
let lastPath = paths.pop(), | ||
current = obj, | ||
i; | ||
for (i = 0; i < paths.length; ++i) { | ||
if (Object.prototype.hasOwnProperty.call(current, paths[i])) { | ||
current = safeGet(current, paths[i]); | ||
} else { | ||
throw new Error('You cant add an observer to an undefined obj'); | ||
} | ||
} | ||
addObserver(current, lastPath, callback); | ||
} | ||
|
||
function deepRemoveObserver(obj, path, callback) { | ||
let paths = path.split('.'); | ||
let lastPath = paths.pop(), | ||
current = obj, | ||
i; | ||
for (i = 0; i < paths.length; ++i) { | ||
if (Object.prototype.hasOwnProperty.call(current, paths[i])) { | ||
current = safeGet(current, paths[i]); | ||
} else { | ||
throw new Error('You cant add an observer to an undefined obj'); | ||
} | ||
} | ||
removeObserver(current, lastPath, callback); | ||
} | ||
|
||
export class EmberChangeset extends BufferedChangeset { | ||
@tracked _changes; | ||
@tracked _errors; | ||
@tracked _content; | ||
@tracked _virtualMirror = {}; | ||
|
||
isObject = isObject; | ||
|
||
|
@@ -56,6 +128,11 @@ export class EmberChangeset extends BufferedChangeset { | |
return safeSet(obj, key, value); | ||
} | ||
|
||
@dependentKeyCompat | ||
get mirror() { | ||
return this[VIRTUAL_MIRROR]; | ||
} | ||
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. Is this meant to be accessed anywhere? I don't see 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. Mainly for public access, for the classic computeds case |
||
|
||
/** | ||
* @property isValid | ||
* @type {Array} | ||
|
@@ -110,7 +187,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 +200,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 +210,26 @@ export class EmberChangeset extends BufferedChangeset { | |
* Returns value or error | ||
*/ | ||
_setProperty({ key, value, oldValue }) { | ||
super._setProperty({ key, value, oldValue }); | ||
let changes = this[CHANGES]; | ||
let mirror = this[VIRTUAL_MIRROR]; | ||
|
||
this.setDeep(mirror, key, true, { | ||
safeSet: this.safeSet, | ||
}); | ||
|
||
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[VIRTUAL_MIRROR], key); | ||
notifyPropertyChange(this, 'changes'); | ||
} | ||
|
||
/** | ||
|
@@ -149,7 +243,7 @@ export class EmberChangeset extends BufferedChangeset { | |
_notifyVirtualProperties(keys) { | ||
keys = super._notifyVirtualProperties(keys); | ||
|
||
(keys || []).forEach((key) => notifyPropertyChange(this, key)); | ||
(keys || []).forEach((key) => deepNotifyPropertyChange(this[VIRTUAL_MIRROR], key)); | ||
|
||
return; | ||
} | ||
|
@@ -159,9 +253,7 @@ export class EmberChangeset extends BufferedChangeset { | |
*/ | ||
_deleteKey(objName, key = '') { | ||
const result = super._deleteKey(objName, key); | ||
|
||
notifyPropertyChange(this, key); | ||
|
||
deepNotifyPropertyChange(this[VIRTUAL_MIRROR], key); | ||
return result; | ||
} | ||
|
||
|
@@ -188,6 +280,26 @@ export class EmberChangeset extends BufferedChangeset { | |
|
||
return this; | ||
} | ||
|
||
get(key) { | ||
safeGet(this[VIRTUAL_MIRROR], key); //consume tag for native tracked dependencies | ||
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. Is this meant to be 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. I followed the pattern of using this[KEYWORD] on internal usages, the get mirror(){} is meant to be used by client code, public access, like.... get data(){} |
||
return super.get(...arguments); | ||
} | ||
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. Does this handle the 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. I think yes? Because of the Proxy traps all access 🤔 |
||
|
||
addObserver(key, callback) { | ||
if (!deepHasOwnProperty(this[VIRTUAL_MIRROR], key)) { | ||
this.setDeep(this[VIRTUAL_MIRROR], key); | ||
} | ||
|
||
deepAddObserver(this[VIRTUAL_MIRROR], key, callback); | ||
} | ||
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. So is 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. Manually adding observers in general is not that common, I needed them for a very specific use case that I'm not entirely sure if it could be solved by native getters and this mirror approach, still having a way to add them gives more flexibility and an actual way to do it, which doesn't exist today, also... adding them to the |
||
|
||
removeObserver(key, callback) { | ||
if (!deepHasOwnProperty(this[VIRTUAL_MIRROR], key)) { | ||
this.setDeep(this[VIRTUAL_MIRROR], key); | ||
} | ||
deepRemoveObserver(this[VIRTUAL_MIRROR], key, callback); | ||
} | ||
} | ||
|
||
/** | ||
|
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.
I think we would want both? Or is the tracked-built-ins intended for things like
@tracked _changes;
as well?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.
🤔 not sure, TBH! Gonna have a look