Inconsistency when comparing simple objects and class instances with getters and private JS fields

[PSanetra]

In the follwing tests the first test fails and the second test succeeds:

import { circularDeepEqual } from 'fast-equals';

class ClassWithPrivateJsField {
  #_a: string;

  constructor(a: string) {
    this.#_a = a;
  }

  get a() {
    return this.#_a;
  }
}

describe('circularDeepEqual', () => {

  // This test fails!
  it('should not consider class instances with different js-private field values via getter to be equal', () => {
    const a = new ClassWithPrivateJsField('v1');
    const b = new ClassWithPrivateJsField('v2');
    expect(circularDeepEqual(a, b)).toBe(false);
  });

  it('should not consider object instances with getter values to be equal', () => {
    expect(
      circularDeepEqual(
        {
          get a() {
            return 'v1';
          },
        },
        {
          get a() {
            return 'v2';
          },
        }
      )
    ).toBe(false);
  });
});

Also see the following stackblitz: https://stackblitz.com/edit/node-hw8ssy?file=index.spec.ts

As a developer this behavior seems to be inconsistent and I would expect that the first test also considers those class instances to be inequal.

Note: I guess the following code is the cause of this issue:

fast-equals/src/objects.ts

Line 22 in 342d9a1

const keysA = Object.keys(a);

Both for (const key in obj) and generally Object.keys(obj) will enumerate the getter keys of simple objects, but not those of class instances.

Also note: jest suffers of the same issue: jestjs/jest#13535

[planttheidea]

You and I start to have different definition of "simple". 🙂 Objects with getters that have impure results is pretty rare, hence why we (and have other comparators, including jest as you call out) don't support it by default. I'll say two things, though.

1. I'll be attempting to add something in the next major to support this. In fast-copy handling for this is available as a "strict" option ... understandably slower, but still possible, which is important.

2. This is why createCustomEqual / createCustomCircularEqual exists as an escape hatch ... for these kind of edge cases you can create a custom comparator so that at least it was possible.

I'm sorry I don't have much better to offer right now, but it is something of an intentional decision to target the primary use case for speed, which has a trade off of not handling edge cases like this. Hopefully in the future, though, I have a better answer.

[PSanetra]

@planttheidea Yes, by simple I meant objects, that are not instances of any classes, but just plain javascript objects with getter properties 😄

I agree, it is definitely not easy to fix.

I discovered this issue during debugging a test that should fail, but didn't. I was comparing CloudEvent instances from the CloudEvents JavaScript SDK. The CloudEvents class contains a private #_data field, that is accessed via a getter from the outside: https://github.com/cloudevents/sdk-javascript/blob/main/src/event/cloudevent.ts#L36

An additional pitfall here was that the TypeScript interface just exposes the data field as a simple field. The getter is just an implementation detail of the class, but breaks equality comparison.

[planttheidea]

It looks to be more systemic than I thought; the reason the property is not discoverable (even with accessors like Object.getOwnPropertyNames() or Object.getOwnPropertyDescriptors()) is because it is not actually on the own object ... the getter is on the prototype of the class. This means that introspection of that object is extremely costly, if even reasonably possible.

However, that is only from a generic usage. Specific use-cases can have specific solutions, and this is definitely a specific use-case! Since the properties in question are known (I'm assuming its the data property in the link), you can create a custom equality comparator with the areObjectsEqual override to look for that specific property:

const circularDeepEqual = createCustomCircularEqual(({ areObjectsEqual }) => ({
  areObjectsEqual: (a, b, isEqual, meta) => areObjectsEqual(a, b, isEqual, meta) && isEqual(a.data, b.data., meta);
}));

This is spitballing a bit, but something like this should give you what you want for your specific use-case. It is highly specific to the contract, so you might want to guard a bit:

const circularDeepEqual = createCustomCircularEqual(({ areObjectsEqual }) => ({
  areObjectsEqual: (a, b, isEqual, meta) => {
    if (!areObjectsEqual(a, b, isEqual, meta)) {
      return false;
    }

    const aCloudEvent = a instanceof CloudEvent;
    const bCloudEvent = b instanceof CloudEvent;

    if (aCloudEvent || bCloudEvent) {
      return aCloudEvent && bCloudEvent && isEqual(a.data, b.data, meta);
    }

    return true;
  };
}));

The contract for createCustomEqual will be a little different in v5 (actively being worked on), but you might be able to get what you need out of v4. This is about the best you'll get, I'm afraid. :(

[PSanetra]

@planttheidea thank you for looking into this. My specific use case was fixed by removing the usage of private fields, but I guess there might be other cases coming where similar issues occur, when private fields are more widely in use.

Thank you for proposing a solution 👍

[planttheidea]

I don't foresee a solution to this on the horizon, as this is related to aspects of the language which are difficult at minimum to work around. Therefore, I'm closing this issue. However, if in the future you find an example of a working implementation in the wild (example: jest fixes the issue you reference above), feel free to reopen and I can take another look.