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

💥 Fix complex deep compose with keepNull=false #9

Merged
merged 1 commit into from
Aug 3, 2023

Commits on Aug 3, 2023

  1. 💥 Fix complex deep compose with keepNull=false

    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:
    
    ```js
    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:
    
    ```js
    AttributeMap.compose(
      {},
      {complex: {foo: {}, bar: 123}},
      false,
    )
    ```
    
    You get:
    
    ```js
    {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.
    alecgibson committed Aug 3, 2023
    Configuration menu
    Copy the full SHA
    8214d29 View commit details
    Browse the repository at this point in the history