[enhance] Merge changes to resource instances

[skoging]

Adresses #9

lodash merge only recursively merges arrays and plain objects, not class instances.

Use lodash mergeWith and a customizer that can handle instances of Resource.

The resourceCustomizer function will merge Resources recursively.

[ntucker]

Can you add tests for your use case? Thanks for the pr!

[skoging]

Already on it 👍

[ntucker]

  static fromJS<T extends typeof Resource>(
    this: T,
    props: Partial<AbstractInstanceType<T>>,
    overrides?: Partial<AbstractInstanceType<T>>,
  ) {
    if (this === Resource)
      throw new Error('cannot construct on abstract types');
    // we type guarded abstract case above, so ok to force typescript to allow constructor call
    const instance = new (this as any)(props) as AbstractInstanceType<T>;
    Object.assign(instance, props, overrides);
    // to trick normalizr into thinking we're Immutable.js does it doesn't copy
    Object.defineProperty(instance, '__ownerID', {
      value: 1337,
      writable: false,
    })
    return instance;
  }
  merge<T extends typeof Resource>(this: AbstractInstanceType<T>, other: Partial<AbstractInstanceType<T>>) {
    return (this.constructor as T).fromJS(this, other);
  }

[ntucker]

actually may be bad idea to add any instance methods as they could potentially conflict with a member. If you just change fromJS tho you could use that to merge yourself

[ntucker]

I'm excited for this change; is there anything else I can help with unblocking?

[skoging]

I went with a new static method on Resource that handles merging. To make this easy to customize on Resources, it accepts an predicate as an optional third argument:

shouldOverrideField: (fieldname: string, newValue: any, oldValue: any) => boolean

The default implementation will override if newValue is not undefined and not strictly equal to oldValue.

I would appreciate some feedback on this. If the approach is acceptable I'll add some documentation as well.

[ntucker]

Can you provide an example use case for needing such merge logic customization? In my use cases a simple one-overrides-the-other-if-it-exists has been sufficient. I try to keep things simple if possible, but of course sometimes things need to be a bit more complicated.

[skoging]

I think the biggest issue here is that the simple one-overrides-the-other-if-it-exists (in the response) logic simple isn't possible to implement as I see it. Once the response has been normalized and entities has been constructed, the information about what fields were present in the response is lost. What we're left with is the default values specified for the resource fields, or undefined for fields that have no default value.

By default only fields that are undefined after the normalized entity has been constructed will be overriden with the value from the existing entity. This means merging only works for fields that have no default value.

I'm not sure how best to handle this, so I figure at least allowing some form of customization is needed.

Additionally there is a strict equals check between the old and new value, so that we can keep referential equality in the cases where no field has changed in the new entity.

[ntucker]

Ahh, that's a good point. So if I add the capability to tell when a default is set, you'd go with the simple merge strategy?

[skoging]

Yeah, that would solve all reasonable use cases that I can think of.

[ntucker]

Landed a2c4793 which exposes a new merge() method that only copies defined members. Tell me if there's anything else I can do :)

[ntucker]

Moving to request changes to rebase on master with 'merge'

[skoging]

I'll get on it. With the merging logic added to Resource, this should be a relatively small change.

[ntucker]

This is looking pretty good, just a few minor things. No pressure tho; but I'm excited to have this in :)

[ntucker]

Awesome work!