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

Implement support for Arrays #104

Merged
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
29 commits
Select commit Hold shift + click to select a range
a96cabe
demonstrate failing scenario with setting array elements
NullVoxPopuli Feb 12, 2021
40ea9ff
update array test for when there is previously no hist as to the type…
NullVoxPopuli Feb 12, 2021
0f40a3d
add more test cases in set-deep.test
NullVoxPopuli Feb 12, 2021
5130a7b
enable setDeep to work with arrays
NullVoxPopuli Feb 12, 2021
ef89f4b
Add additional test cases for set-deep
NullVoxPopuli Feb 12, 2021
a0d77c8
add tests and support for arrays in merge deep
NullVoxPopuli Feb 12, 2021
c4f7c3e
remove debuggers
NullVoxPopuli Feb 12, 2021
1f71e59
add more array tests to merge-deep
NullVoxPopuli Feb 12, 2021
a87271e
Add library tests to GH Actions
NullVoxPopuli Feb 16, 2021
8bdfbfa
add debug script alias to package.json because oof
NullVoxPopuli Feb 16, 2021
b312b51
fix typo in workflow file
NullVoxPopuli Feb 16, 2021
372aec3
Add more tests
NullVoxPopuli Feb 16, 2021
989f5c6
Add additional assertions to tests
NullVoxPopuli Feb 16, 2021
87841e9
Add additional tests for arrays of objects (with a failing test which…
NullVoxPopuli Feb 16, 2021
8a1389e
move a tets into the arrays within nested objects describe block
NullVoxPopuli Feb 16, 2021
69c6527
Fix the failing test w/r/t deep nested changes in arrays
NullVoxPopuli Feb 16, 2021
2e518f0
remove jest hacks for testing equality
NullVoxPopuli Feb 17, 2021
c888a42
cleanup
NullVoxPopuli Feb 17, 2021
ad3c0f0
add test for removing items from an array
NullVoxPopuli Feb 17, 2021
94fae67
add another test scenario to support setting entire objects on arrays…
NullVoxPopuli Feb 18, 2021
595fa17
add another test scenario to support setting entire objects on arrays…
NullVoxPopuli Feb 18, 2021
608c833
Fix issue with setting properties on brand new objects within arrays …
NullVoxPopuli Feb 18, 2021
c38e227
uncomment assertion
NullVoxPopuli Feb 18, 2021
0c7a68a
add additional assertions
NullVoxPopuli Feb 19, 2021
9bb6fed
add test cases and a fix for shallowish arrays
NullVoxPopuli Feb 19, 2021
d24716c
remove debugger
NullVoxPopuli Feb 19, 2021
f4d5c22
leaf nodes are no longer correct when there is an array in the path
NullVoxPopuli Feb 19, 2021
6cd8c9d
add additional assertions
NullVoxPopuli Feb 23, 2021
e69073b
fix access to deep sub changes within an array where there is no change
NullVoxPopuli Feb 23, 2021
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
22 changes: 22 additions & 0 deletions .github/workflows/ci.yml
Original file line number Diff line number Diff line change
Expand Up @@ -37,3 +37,25 @@ jobs:
run: npm run lint
- name: test
run: npm run test

compat:
name: Compatibility
env:
CI: true
runs-on: ubuntu-latest
strategy:
matrix:
lib:
- ember-changeset
- ember-changeset-validations

steps:
- uses: actions/checkout@v2
- name: Install node
uses: actions/setup-node@v2-beta
with:
node-version: 12.x
- run: npm install
- name: "Test: ${{ matrix.lib }}"
run: npm run test-external:${{ matrix.lib }}

1 change: 1 addition & 0 deletions package.json
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@
"test": "jest",
"test:all": "npm run test && npm run test-external:ember-changeset && npm run test-external:ember-changeset-validations",
"test.watch": "jest --watch",
"test:debug:named": "node --inspect-brk node_modules/.bin/jest --runInBand --watch --testNamePattern",
"lint": "eslint .",
"prepare": "npm run build",
"prepublishOnly": "npm run test && npm run lint",
Expand Down
21 changes: 19 additions & 2 deletions src/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,7 @@ import {
ValidatorAction,
ValidatorMap
} from './types';
import { isArrayObject } from './utils/array-object';
Copy link
Collaborator

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

Copy link
Contributor Author

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?


export {
CHANGESET,
Expand Down Expand Up @@ -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;
}
Expand Down Expand Up @@ -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);
Copy link
Collaborator

Choose a reason for hiding this comment

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

What is the value of result here for the array case? I thought one of the if expressions above would have been tripped. It looks like there might be some minor refactors here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

result is undefined here. It happens mostly when working with new array entries or previously deleted entries, iirc


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;
Copy link
Collaborator

Choose a reason for hiding this comment

The 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 ObjectTreeNode is to ensure we don't lose sibling keys. So an object with both changes and unchanged content are able to be retrieved at any arbitrary level of the object.

{
  changeValue: ...,
  original: ... 
}

Now for the array scenario, in one case we have changes we want to return. In the other, we have content we want to return. However, in both cases, I imagine that the type is an array and we don't have to deal with merging sibling keys.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

For the array case, do we need to return an ObjectTreeNode?

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 changeset.get('prop.someArray') where it now needs unwrap, where it previously did not -- but this is more consistent with the deep object changes.

. However, in both cases, I imagine that the type is an array and we don't have to deal with merging sibling keys.

🤷
The overloaded behavior of this method is dizzying to me :(

}
}

// this comes after the isObject check to ensure we don't lose remaining keys
Expand Down
24 changes: 24 additions & 0 deletions src/utils/array-object.ts
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;
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

very nice!

7 changes: 7 additions & 0 deletions src/utils/merge-deep.ts
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
import Change, { getChangeValue, isChange } from '../-private/change';
import normalizeObject from './normalize-object';
import { isArrayObject, objectToArray, arrayToObject } from './array-object';

interface Options {
safeGet: any;
Expand Down Expand Up @@ -154,6 +155,12 @@ export default function mergeDeep(
let sourceAndTargetTypesMatch = sourceIsArray === targetIsArray;

if (!sourceAndTargetTypesMatch) {
let sourceIsArrayLike = isArrayObject(source);

if (targetIsArray && sourceIsArrayLike) {
return objectToArray(mergeTargetAndSource(arrayToObject(target), source, options));
}

return source;
} else if (sourceIsArray) {
return source;
Expand Down
11 changes: 11 additions & 0 deletions src/utils/object-tree-node.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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 = {
/**
Expand All @@ -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;
}
Expand Down Expand Up @@ -118,11 +121,19 @@ class ObjectTreeNode implements ProxyHandler {
if (isObject(content)) {
changes = normalizeObject(changes, this.isObject);
return { ...content, ...changes };
} else if (Array.isArray(content)) {
Copy link
Collaborator

Choose a reason for hiding this comment

The 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 isArray.

changes = normalizeObject(changes, this.isObject);

return objectToArray(mergeDeep(arrayToObject(content), changes));
}
}

return changes;
}

toJSON() {
return JSON.parse(JSON.stringify(this.unwrap()));
}
}

export { ObjectTreeNode };
27 changes: 22 additions & 5 deletions src/utils/set-deep.ts
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;
Expand Down Expand Up @@ -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])) {
Copy link
Collaborator

Choose a reason for hiding this comment

The 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 target[prop] might be an array? Another way to say it - would getChangeValue below work with a type of array?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Are you indicating target[prop] might be an array?

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
Copy link
Contributor Author

@NullVoxPopuli NullVoxPopuli Feb 18, 2021

Choose a reason for hiding this comment

The 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;
Expand Down
Loading