💥 Fix complex deep compose with keepNull=false

[alecgibson]

This is a BREAKING CHANGE.

We already did some work on this in #8

At the moment, our .compose() behaviour for complex attributes doesn't act consistently between root and deep attributes, which wasn't the intended behaviour originally.

The merge behaviour is meant to be based on the root behaviour of the "non-complex" attributes. That is:

expect(
  AttributeMap.compose({}, {}, false)
).toEqual(undefined)

expect(
  AttributeMap.compose({}, {foo: null}, false)
).toEqual(undefined)

In other words, with keepNull=false:

• an object whose only values are nullish is treated as empty
• an empty object resolves to undefined when composed

However, at the moment we don't display this behaviour for deeply- nested objects.

If the two statements above are followed, then with complex attributes and keepNull=false, we would:

• never expect to see a nullish value
• never expect to see an empty object

However at the moment, if you do:

AttributeMap.compose(
  {},
  {complex: {foo: {}, bar: 123}},
  false,
)

You get:

{complex: {foo: {}, bar: 123}}

But we said that an empty object - foo - should be treated as undefined, so should have been removed in the compose().

The issue was that our root-level attributes were treated differently to deeper attributes: we only checked and removed root-level attributes if they were "deep null", but didn't remove ancestors using the same condition, which led to this inconsistency.

This change fixes this issue by simplifying our keepNull=false logic and performing a post-order depth-first tree traversal, which first clears out descendant objects of nullish values, and only then checks if a given node has an empty or nullish value itself. All levels of the tree are treated the same, and should therefore have consistent behaviour.

Even though this was unintended behaviour and is a bugfix, this change is marked as BREAKING because it changes the behaviour of .compose(), and therefore changes the shape of resulting documents, potentially by stripping out empty objects that may or may not have been relied on previously.

[dawidreedsy]

I would for sure change the name keepNull, as {} is not null.

The second thing is that empty object is an truthful value, so it may lead to some unexpected behaviours like, some imaginary

{
   `comment`: {
         showExtraInfo: {
            withBar?: boolean
         }
    }
}

Now if we want to display the box for extra info we would check for !!comment.showExtraInfo, even though withBar might be undefined. This is more like something to consider, as TBH I cannot think of real world example where the empty object would represent some valuable information.

Worth noting is that delta only works on the root level properties, so that

compose({complex: {foo: 1, bar:2}}, {complex: {foo:{tar: null}}}, false)

would return

{ complex: { bar: 123 } }

so it is not really that weird that we operate only on root level.

TL;TR This is the stuff worth mentioning, but as you marked is as breaking change, then I guess it is fine, apart from changing the name of keepNull

[alecgibson]

@dawidreedsy

delta only works on the root level properties

I'm not sure I understand this statement?

I agree with you on principle about changing the name of keepNull, but I think we should keep it as-is in order to stay as close to upstream as possible.

[dawidreedsy]

Ignore my comment about delta only works on the root level properties. I did wrong test 😓