-
-
Notifications
You must be signed in to change notification settings - Fork 27
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
Implement support for Arrays #104
Changes from all commits
a96cabe
40ea9ff
0f40a3d
5130a7b
ef89f4b
a0d77c8
c4f7c3e
1f71e59
a87271e
8bdfbfa
b312b51
372aec3
989f5c6
87841e9
8a1389e
69c6527
2e518f0
c888a42
ad3c0f0
94fae67
595fa17
608c833
c38e227
0c7a68a
9bb6fed
d24716c
f4d5c22
6cd8c9d
e69073b
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 |
---|---|---|
|
@@ -39,6 +39,7 @@ import { | |
ValidatorAction, | ||
ValidatorMap | ||
} from './types'; | ||
import { isArrayObject } from './utils/array-object'; | ||
|
||
export { | ||
CHANGESET, | ||
|
@@ -919,7 +920,9 @@ export class BufferedChangeset implements IChangeset { | |
let content: Content = this[CONTENT]; | ||
if (Object.prototype.hasOwnProperty.call(changes, baseKey)) { | ||
const changesValue = this.getDeep(changes, key); | ||
if (!this.isObject(changesValue) && changesValue !== undefined) { | ||
const isObject = this.isObject(changesValue); | ||
|
||
if (!isObject && changesValue !== undefined) { | ||
// if safeGet returns a primitive, then go ahead return | ||
return changesValue; | ||
} | ||
|
@@ -955,12 +958,26 @@ export class BufferedChangeset implements IChangeset { | |
const baseContent = this.safeGet(content, baseKey); | ||
const subContent = this.getDeep(baseContent, remaining.join('.')); | ||
const subChanges = getSubObject(changes, key); | ||
// give back and object that can further retrieve changes and/or content | ||
// give back an object that can further retrieve changes and/or content | ||
const tree = new ObjectTreeNode(subChanges, subContent, this.getDeep, this.isObject); | ||
return tree.proxy; | ||
} else if (typeof result !== 'undefined') { | ||
return result; | ||
} | ||
const baseContent = this.safeGet(content, baseKey); | ||
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. What is the value of 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 (Array.isArray(baseContent)) { | ||
const subChanges = getSubObject(changes, key); | ||
|
||
if (!subChanges) { | ||
return this.getDeep(baseContent, remaining.join('.')); | ||
} | ||
|
||
// give back an object that can further retrieve changes and/or content | ||
const tree = new ObjectTreeNode(subChanges, baseContent, this.getDeep, this.isObject); | ||
|
||
return tree; | ||
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. For the array case, do we need to return an ObjectTreeNode? Just a little background just in case there is some intersting information for this discussion... The main reason for the
Now for the array scenario, in one case we have 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, cause atm, ObjectTreeNode is the only thing that knows that changes are on arrays and can convert between arrays and array objects. however this changes the behavior of
🤷 |
||
} | ||
} | ||
|
||
// this comes after the isObject check to ensure we don't lose remaining keys | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,24 @@ | ||
export function isArrayObject(obj: Record<string, any>) { | ||
if (!obj) return false; | ||
|
||
let maybeIndicies = Object.keys(obj); | ||
|
||
return maybeIndicies.every(key => Number.isInteger(parseInt(key, 10))); | ||
} | ||
|
||
export function arrayToObject(array: any[]): Record<string, any> { | ||
return array.reduce((obj, item, index) => { | ||
obj[index] = item; | ||
return obj; | ||
}, {} as Record<string, any>); | ||
} | ||
|
||
export function objectToArray(obj: Record<string, any>): any[] { | ||
let result: any[] = []; | ||
|
||
for (let [index, value] of Object.entries(obj)) { | ||
result[parseInt(index, 10)] = value; | ||
} | ||
|
||
return result; | ||
} | ||
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. very nice! |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -3,6 +3,8 @@ import isObject from './is-object'; | |
import setDeep from './set-deep'; | ||
import Change, { getChangeValue, isChange } from '../-private/change'; | ||
import normalizeObject from './normalize-object'; | ||
import { objectToArray, arrayToObject, isArrayObject } from './array-object'; | ||
import mergeDeep from './merge-deep'; | ||
|
||
const objectProxyHandler = { | ||
/** | ||
|
@@ -18,6 +20,7 @@ const objectProxyHandler = { | |
|
||
if (node.changes.hasOwnProperty && node.changes.hasOwnProperty(key)) { | ||
childValue = node.safeGet(node.changes, key); | ||
|
||
if (typeof childValue === 'undefined') { | ||
return; | ||
} | ||
|
@@ -118,11 +121,19 @@ class ObjectTreeNode implements ProxyHandler { | |
if (isObject(content)) { | ||
changes = normalizeObject(changes, this.isObject); | ||
return { ...content, ...changes }; | ||
} else if (Array.isArray(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. This seems safe for ember-data. B/c we aren't using the Ember provided |
||
changes = normalizeObject(changes, this.isObject); | ||
|
||
return objectToArray(mergeDeep(arrayToObject(content), changes)); | ||
} | ||
} | ||
|
||
return changes; | ||
} | ||
|
||
toJSON() { | ||
return JSON.parse(JSON.stringify(this.unwrap())); | ||
} | ||
} | ||
|
||
export { ObjectTreeNode }; |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,5 +1,6 @@ | ||
import Change, { getChangeValue, isChange } from '../-private/change'; | ||
import isObject from './is-object'; | ||
import { isArrayObject } from './array-object'; | ||
|
||
interface Options { | ||
safeSet: any; | ||
|
@@ -59,18 +60,34 @@ export default function setDeep( | |
for (let i = 0; i < keys.length; i++) { | ||
let prop = keys[i]; | ||
|
||
if (Array.isArray(target) && parseInt(prop, 10) < 0) { | ||
throw new Error( | ||
'Negative indices are not allowed as arrays do not serialize values at negative indices' | ||
); | ||
} | ||
|
||
const isObj = isObject(target[prop]); | ||
if (!isObj) { | ||
const isArray = Array.isArray(target[prop]); | ||
const isComplex = isObj || isArray; | ||
|
||
if (!isComplex) { | ||
options.safeSet(target, prop, {}); | ||
} else if (isObj && isChange(target[prop])) { | ||
} else if (isComplex && isChange(target[prop])) { | ||
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. Initially, I thought this arm would be for a Change only - which would have an object shape. Are you indicating 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.
yup, the object-array thing of today works with array's index access, which is cool. thanks implicit type casting! (for reals tho, this is one of the few times where I like this part of JS lol) |
||
let changeValue = getChangeValue(target[prop]); | ||
|
||
if (isObject(changeValue)) { | ||
// if an object, we don't want to lose sibling keys | ||
const siblings = findSiblings(changeValue, keys); | ||
|
||
const resolvedValue = isChange(value) ? getChangeValue(value) : value; | ||
target[prop] = new Change( | ||
setDeep(siblings, keys.slice(1, keys.length).join('.'), resolvedValue, options) | ||
); | ||
const nestedKeys = | ||
Array.isArray(target) || isArrayObject(target) | ||
? keys.slice(i + 1, keys.length).join('.') // remove first key segment as well as the index | ||
: keys.slice(1, keys.length).join('.'); // remove first key segment | ||
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 don't know if the object case needs to also be based off the index, but for the ArrayLike situation, the dynamic slice made sense |
||
|
||
const newValue = setDeep(siblings, nestedKeys, resolvedValue, options); | ||
|
||
target[prop] = new Change(newValue); | ||
|
||
// since we are done with the `path`, we can terminate the for loop and return control | ||
break; | ||
|
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.
✂️
Should make sure this is caught by linting in CI
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 can PR tomorrow that adds / updates linting in the CI config tomorrow?