Implement support for Arrays

[NullVoxPopuli]

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

[NullVoxPopuli]

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?

[NullVoxPopuli]

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 :upside_down_face: ) -- I can never remember all the args needed to debug with jest

[NullVoxPopuli]

also, should this scenario be supported?

      it('can add a new object at once, and edit it', () => {
        const changeset = Changeset(initialData);

        changeset.set('contact.emails.1', {
          funEmail: 'fun@email.com',
          primary: 'primary@email.com'
        });

        expect(changeset.get('contact.emails.1.funEmail')).toEqual('fun@email.com');
        expect(changeset.get('contact.emails.1.primary')).toEqual('primary@email.com');
        expect(changeset.get('contact.emails').unwrap()).toEqual([
          { primary: 'bob@email.com' },
          { primary: 'primary@email.com', funEmail: 'fun@email.com' }
        ]);
        expect(changeset.changes).toEqual([
          {
            key: 'contact.emails.1',
            value: { funEmail: 'fun@email.com', primary: 'primary@email.com' }
          }
        ]);

        changeset.set('contact.emails.1.primary', 'primary2@email.com');

        expect(changeset.get('contact.emails.1.primary')).toEqual('primary2@email.com');
      });

  ● Unit | Utility | changeset › arrays of objects within nested objects › #set › can add a new object at once, and edit it

    expect(received).toEqual(expected) // deep equality

    Expected: "primary2@email.com"
    Received: undefined

       997 |         changeset.set('contact.emails.1.primary', 'primary2@email.com');
       998 | 
    >  999 |         expect(changeset.get('contact.emails.1.primary')).toEqual('primary2@email.com');
           |                                                           ^
      1000 |       });
      1001 |     });
      1002 |   });

[NullVoxPopuli]

looks like yes, based on other tests (#execute keys prototype of set object and #execute it works with nested keys'

[NullVoxPopuli]

@snewcomer We should be good to go :+1:

[NullVoxPopuli]

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]

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]

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]

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. :thinking:

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]

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?