Implement support for Arrays #104

NullVoxPopuli · 2021-02-12T18:14:59Z

Repro of some weird behavior: https://runkit.com/nullvoxpopuli/validated-changeset-with-deep-arrays

I have a complex object with objects as array entries that I'd like to manage with a changeset. :D

the example I have at work is:

{
  // big object
  listOfStuff: [
     {
       ... stuff
     },
     {
        ... some stuff
     } 
   ]
}

I suppose creating arrays when the property is undefined doesn't really make sense, because you can't know if the user does indeed want an array, or a if they want an object with numbered keys that would behave kinda like an array.

I propose that if a value is an array, we set at indicies, rather than properties on an object

… of the object

NullVoxPopuli · 2021-02-12T23:33:40Z

Not sure how you feel about the approach I took here, but the gist is that merging arrays by index is weird -- we need to convert the arrays to objects with index-keys, making the objects "ArrayLike" so that we can merge two objects, rather than two arrays, which we already know how to do. I added some utilities in a new file to help out with this.

Some outstanding questions:

should changeset.get('some.path.to.array') always return an array after setting items on the array? example test
should the representation of the set of changes be changed to an array to always match the (sub)content type? we need to convert this to an object anyway for merging purposes -- would it be better to leave it as an object, only converting back to an array as it's read?

snewcomer

Thanks for this PR! I think I am generally in favor of this PR. A few more assertions to battle test this would be helpful.

Note we also have some preliminary checks before merging you can run locally - npm run test:ember-changeset && npm run test:ember-changeset-validations

test/index.test.ts

snewcomer · 2021-02-13T19:34:41Z

src/utils/set-deep.ts

      options.safeSet(target, prop, {});
-    } else if (isObj && isChange(target[prop])) {
+    } else if (isComplex && isChange(target[prop])) {


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?

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)

snewcomer · 2021-02-13T19:35:23Z

src/utils/array-object.ts

+  }
+
+  return result;
+}


snewcomer · 2021-02-14T17:33:46Z

test/index.test.ts

+    expect(dummyChangeset.get('contact.emails')).toEqual(['bob@email.com', 'the_bob@email.com']);
+
+    dummyChangeset.set('contact.emails.0', 'fred@email.com');
+    dummyChangeset.set('contact.emails.1', 'the_fred@email.com');


Could we have an assertion where get/set is out of range?

on the lower bound, out of range is only negative, I can add tests for that, but on the upper range, there is no out of bounds, because this is how you'd add stuff.

Oh right. But do you think it is worthwhile adding a test for this custom implementation when contact.emails is only len 2 but we do the following?

dummyChangeset.set('contact.emails.10', 'the_fred@email.com');

yup! I'm working on adding more tests now (as soon as I figure out how to make changeset.get('some array').toEqual(some array)

TIL

I think this should probs be a hard error? even though behavior is kinda undefined in native arrays?

test/index.test.ts

NullVoxPopuli · 2021-02-16T18:49:37Z

Also added:

library tests to CI
yarn test:debug:named "part of test name here" (or I suppose npm run, if that's what you're in to 🙃 ) -- I can never remember all the args needed to debug with jest

NullVoxPopuli · 2021-02-16T22:48:09Z

src/utils/object-tree-node.ts

@@ -11,13 +12,24 @@ const objectProxyHandler = {
   */
  get(node: ProxyHandler, key: string): any {
    if (typeof key === 'symbol') {
+      if (key === Symbol.iterator) {


this feels fishy -- all of this is to make jest's expect work... which I feel like is kinda lying -- cause if you console.log(changeset.get('some array'), you get the ObjectNodeProxy, which.. I think is fine and it makes sense that you'd have to call unwrap on it to get the human-readable value out?

test/index.test.ts

… uncovers a bug)

test/index.test.ts

snewcomer

This is looking pretty sound!

src/utils/merge-deep.ts

test/index.test.ts

snewcomer · 2021-02-17T03:16:38Z

src/utils/object-tree-node.ts

@@ -118,11 +131,19 @@ class ObjectTreeNode implements ProxyHandler {
      if (isObject(content)) {
        changes = normalizeObject(changes, this.isObject);
        return { ...content, ...changes };
+      } else if (Array.isArray(content)) {


This seems safe for ember-data. B/c we aren't using the Ember provided isArray.

NullVoxPopuli · 2021-02-17T23:54:35Z

test/index.test.ts

+
+        changeset.set('contact.emails.1', null);
+
+        expect(changeset.get('contact.emails').unwrap()).toEqual([null, null]);


idk if this is technically "deleting", but it made me wonder if changeset needs some post-processing callback to run before "save".

cause for some uses, you may want to collapse all the falsey values in an array, like,

data.array = data.array.filter(Boolean);

but I could also see some use cases where you'd want the falsey values

I think in this case, the recommended way would be to .set('contact.emails', [])? I'm happy leaving the nulls in there if that is what the set did!

… at once

…and array-like objects

NullVoxPopuli · 2021-02-18T19:05:41Z

@snewcomer We should be good to go 👍

NullVoxPopuli · 2021-02-18T19:06:18Z

src/utils/set-deep.ts

+        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


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

NullVoxPopuli · 2021-02-18T19:20:44Z

test/utils/set-deep.test.ts

@@ -228,6 +228,112 @@ describe('Unit | Utility | set deep', () => {
    });
  });

+  it('set on nested properties within an empty object', () => {


I had to add this test, because my intuitive feelings about how arrays should work wasn't working.

idk if this is intended, or why it would be intended, but intuitively, I would have thoughts this would have been the case:

// ... expect(value).toEqual({ top: { name: new Change('zoo'), foo: new Change({ other: 'baz' }) } });

because the foo is an entirely new object,

and how this correlates to arrays:
instead of

it('sets when the array is initially empty', () => { const objA = { top: [] }; let value = setDeep(objA, 'top.0.name', new Change('zoo')); expect(value).toEqual({ top: [{ name: new Change('zoo') }] }); value = setDeep(value, 'top.0.foo.other', new Change('baz')); expect(value).toEqual({ top: [ { name: new Change('zoo'), foo: { other: new Change('baz') } } ] }); });

maybe:

it('sets when the array is initially empty', () => { const objA = { top: [] }; let value = setDeep(objA, 'top.0.name', new Change('zoo')); expect(value).toEqual({ top: [new Change({ name: 'zoo' })] }); value = setDeep(value, 'top.0.foo.other', new Change('baz')); // the big difference here -- the first entry remains an entire single Change expect(value).toEqual({ top: [ new Change({ name: 'zoo', foo: { other: 'baz' } }) ] }); });

NullVoxPopuli · 2021-02-18T19:22:27Z

test/utils/set-deep.test.ts

+    });
+  });
+
+  describe('arrays at various nestings within an object', () => {


I've started using describe blocks for groups of this stuff so I can debug scoped down sections of the test suite, so:

yarn test:debug:named "arrays at various nestings within an object"

allows me to debug just this subset of tests

snewcomer

This is a really really nice PR. Thank you and let me know when you are comfortable for me to merge and release!

test/index.test.ts

snewcomer · 2021-02-19T14:00:56Z

test/index.test.ts

+
+        changeset.set('contact.emails.1', null);
+
+        expect(changeset.get('contact.emails').unwrap()).toEqual([null, null]);


I think in this case, the recommended way would be to .set('contact.emails', [])? I'm happy leaving the nulls in there if that is what the set did!

NullVoxPopuli · 2021-02-19T14:07:50Z

Yeah, I'm good with merging. I think at this point, I need to get our internal work merged (pending releases of this, ember changeset, and ember validated changeset).

Anything I run in to, I can open bugfix prs for

🎉

NullVoxPopuli · 2021-02-19T18:39:27Z

I found an inconsistency depending on the depth of a change:

changeset.get('contacts.emails') // => requires unwrap
changeset.get('emails') // does not have unwrap

I have a use case for wanting to access non-leaf data: iterating over the data to display navigation to different forms that would open a form to edit each array entry (reason being that when generating links and form fields, the index needs to be correct) -- so...
I don't think we should block this PR, as it's gotten kinda big, but, I do worry than forcing changeset.get to return a that node proxy for all complex objects could be a breaking change

I'm now wondering if the implemented approach is correct at all?
maybe when a changeset is created, all arrays are converted to objectArrays, and then we only deal with conversion on save and get?

I'm might open another PR and point it at the array-entry-setting branch to keep the ideas isolated. The main thing I need to solve right now, which I was suprised to find doesn't work with the current implementation is top level arrays as a value of some top level property, example: { prop: [] }

NullVoxPopuli · 2021-02-19T19:11:40Z

Turns out, I'm getting faster at this :D

We should be good to go again :D
No breaking change needed, since the added proxy wrap in get is specifically for arrays

NullVoxPopuli · 2021-02-19T22:17:28Z

I just keep finding issues. O.o

@snewcomer I added another test for what I'm running in to now. Idk if you want to take a stab at it.

But I wonder if some refactoring might need to happen in order to make this easier?
I feel like this should be an easy addition due to how similar array and object access is -- but it hasn't been. 🤔

We also need a consistent way to get the "current value of the subtree at 'this' path", like.. I was thinking maybe a preview method that also takes a property path but always returns the source data merged with the changes known for that path.

My recent issues have come from top level properties having inconsistent need for unwrap. Example:

have emails: [], can access via changeset.get('emails');
make change to an item within that array
changeset.get('emails') now needs unwrap() (only because I added an Array.isArray check in get to return a proxy) -- but without it, there is no merge of data and changes. It seems only the ObjectTreeNode can give a preview.

idk what the consequences of this are yet, but:
https://github.com/NullVoxPopuli/validated-changeset/pull/1/files

all but one test passes -- there might be a deep object merge bug? I need to look in to that
I'd need to convert back to an array no get, which brings me back to needing to differentiate between internal get vs preview get.

Maybe we can keep get, but then switch to a secret method so that userspace isn't affected, and use that for when we want "Change" values.

NullVoxPopuli · 2021-02-22T20:29:41Z

Just found out there is something in this PR that fixes normal object behavior. So maybe it is best to merge it and iterate later? thoughts?

snewcomer · 2021-02-23T04:08:33Z

test/index.test.ts

+          primary: 'primary0@email.com'
+        },
+        null
+      ]);


Does this test pass if you add these assertions (similar to the test below)?

expect(changeset.get('emails.0.fun')).toEqual('fun0@email.com'); expect(changeset.get('emails.0.primary')).toEqual('primary0@email.com');

oh interesting. This is the diff with those:

I think this is a more general bug though, and why my work stuff can't use the published validated changeset.

sometimes when accessing data via get, an object returns.
In this case, the object is just totally incorrect. 🤔

Ok so same as the one below (was just checking if the value was the culprit).

Overall, this is a really great PR and I think we are really close. I think we can focus on .get or ObjectTreeNode may need to be modified. I can certainly jump in and see where the last few improvements need to be made.

I'll take another stab, and see what happens :D

fixed it -- wasn't so bad after all :D

snewcomer

I'm really glad the tests are passing! Just a few questions/comments before we can get this in!

snewcomer · 2021-02-24T03:07:02Z

src/index.ts

@@ -39,6 +39,7 @@ import {
  ValidatorAction,
  ValidatorMap
 } from './types';
+import { isArrayObject } from './utils/array-object';


✂️

Should make sure this is caught by linting in CI

I can PR tomorrow that adds / updates linting in the CI config tomorrow?

snewcomer · 2021-02-24T03:24:19Z

src/index.ts

          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);


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.

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

snewcomer · 2021-02-24T03:47:15Z

src/index.ts

+          // give back an object that can further retrieve changes and/or content
+          const tree = new ObjectTreeNode(subChanges, baseContent, this.getDeep, this.isObject);
+
+          return tree;


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.

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 :(

NullVoxPopuli added 8 commits February 12, 2021 13:11

demonstrate failing scenario with setting array elements

a96cabe

update array test for when there is previously no hist as to the type…

40ea9ff

… of the object

add more test cases in set-deep.test

0f40a3d

enable setDeep to work with arrays

5130a7b

Add additional test cases for set-deep

ef89f4b

add tests and support for arrays in merge deep

a0d77c8

remove debuggers

c4f7c3e

add more array tests to merge-deep

1f71e59

NullVoxPopuli changed the title ~~demonstrate failing scenario with setting array elements~~ Implement support for Arrays Feb 12, 2021

snewcomer reviewed Feb 14, 2021

View reviewed changes

Add library tests to GH Actions

a87271e

NullVoxPopuli force-pushed the array-entry-setting branch from 96e4512 to a87271e Compare February 16, 2021 18:40

add debug script alias to package.json because oof

8bdfbfa

NullVoxPopuli added 2 commits February 16, 2021 13:53

fix typo in workflow file

b312b51

Add more tests

372aec3

NullVoxPopuli commented Feb 16, 2021

View reviewed changes

test/index.test.ts Show resolved Hide resolved

NullVoxPopuli added 4 commits February 16, 2021 17:54

Add additional assertions to tests

989f5c6

Add additional tests for arrays of objects (with a failing test which…

87841e9

… uncovers a bug)

move a tets into the arrays within nested objects describe block

8a1389e

Fix the failing test w/r/t deep nested changes in arrays

69c6527

NullVoxPopuli commented Feb 16, 2021

View reviewed changes

test/index.test.ts Outdated Show resolved Hide resolved

snewcomer reviewed Feb 17, 2021

View reviewed changes

NullVoxPopuli added 2 commits February 17, 2021 12:00

remove jest hacks for testing equality

2e518f0

cleanup

c888a42

NullVoxPopuli requested a review from snewcomer February 17, 2021 17:04

add test for removing items from an array

ad3c0f0

NullVoxPopuli commented Feb 17, 2021

View reviewed changes

add another test scenario to support setting entire objects on arrays…

595fa17

… at once

snewcomer assigned NullVoxPopuli Feb 18, 2021

snewcomer added the enhancement New feature or request label Feb 18, 2021

Fix issue with setting properties on brand new objects within arrays …

608c833

…and array-like objects

NullVoxPopuli commented Feb 18, 2021

View reviewed changes

uncomment assertion

c38e227

NullVoxPopuli commented Feb 18, 2021

View reviewed changes

snewcomer approved these changes Feb 19, 2021

View reviewed changes

add additional assertions

0c7a68a

NullVoxPopuli added 3 commits February 19, 2021 14:11

add test cases and a fix for shallowish arrays

9bb6fed

remove debugger

d24716c

leaf nodes are no longer correct when there is an array in the path

f4d5c22

NullVoxPopuli mentioned this pull request Feb 19, 2021

Demo ignoring the array problem and silently convert to/from objects NullVoxPopuli/validated-changeset#1

Open

snewcomer reviewed Feb 23, 2021

View reviewed changes

NullVoxPopuli added 2 commits February 23, 2021 09:32

add additional assertions

6cd8c9d

fix access to deep sub changes within an array where there is no change

e69073b

snewcomer reviewed Feb 24, 2021

View reviewed changes

snewcomer merged commit 237e63a into adopted-ember-addons:master Feb 24, 2021

snewcomer mentioned this pull request Feb 25, 2021

Refactor: no unwrap for arrays #110

Merged

pernambucano mentioned this pull request Sep 22, 2021

Improved Ember Data and Ember Changeset support for relationships adopted-ember-addons/ember-changeset#599

Open


		changeset.set('contact.emails.1', null);

		expect(changeset.get('contact.emails').unwrap()).toEqual([null, null]);

Implement support for Arrays #104

Implement support for Arrays #104

Conversation

NullVoxPopuli commented Feb 12, 2021 • edited Loading

NullVoxPopuli commented Feb 12, 2021 • edited Loading

snewcomer left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

NullVoxPopuli Feb 16, 2021 • edited Loading

Choose a reason for hiding this comment

NullVoxPopuli commented Feb 16, 2021

Choose a reason for hiding this comment

snewcomer left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

NullVoxPopuli commented Feb 18, 2021

NullVoxPopuli Feb 18, 2021 • edited Loading

Choose a reason for hiding this comment

NullVoxPopuli Feb 18, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

snewcomer left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

NullVoxPopuli commented Feb 19, 2021

NullVoxPopuli commented Feb 19, 2021 • edited Loading

NullVoxPopuli commented Feb 19, 2021

NullVoxPopuli commented Feb 19, 2021 • edited Loading

NullVoxPopuli commented Feb 22, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

snewcomer left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

NullVoxPopuli commented Feb 12, 2021 •

edited

Loading

NullVoxPopuli commented Feb 12, 2021 •

edited

Loading

NullVoxPopuli Feb 16, 2021 •

edited

Loading

NullVoxPopuli Feb 18, 2021 •

edited

Loading

NullVoxPopuli Feb 18, 2021 •

edited

Loading

NullVoxPopuli commented Feb 19, 2021 •

edited

Loading

NullVoxPopuli commented Feb 19, 2021 •

edited

Loading