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

toMatchObject doesn't work well with Proxy objects #6672

Closed
6 tasks done
jimmycallin opened this issue Oct 9, 2024 · 8 comments
Closed
6 tasks done

toMatchObject doesn't work well with Proxy objects #6672

jimmycallin opened this issue Oct 9, 2024 · 8 comments
Labels
p2-to-be-discussed Enhancement under consideration (priority)

Comments

@jimmycallin
Copy link

jimmycallin commented Oct 9, 2024

Describe the bug

It seems like toMatchObject doesn't work well with proxy objects, and won't call the handler to check equality.

Reproduction

  const proxyObj = new Proxy(
    { foo: "bar" },
    {
      get(target, prop, receiver) {
        if (prop === "bar") {
          return "foo";
        }
      },
    },
  );

  // this is equivalent object without proxy
  const obj = { foo: "bar", bar: "foo" };

  // passes correctly
  expect(obj).toMatchObject({ bar: "foo" });
  // passes correctly
  expect(proxyObj.bar).toBe("foo");
  // fails incorrectly
  expect(proxyObj).toMatchObject({ bar: "foo" });
AssertionError: expected { foo: undefined } to match object { bar: 'foo' }
(1 matching property omitted from actual)

- Expected
+ Received

  Object {
-   "bar": "foo",
+   "foo": undefined,
  }

System Info

System:
    OS: macOS 15.0.1
    CPU: (8) arm64 Apple M1
    Memory: 65.80 MB / 16.00 GB
    Shell: 3.7.1 - /opt/homebrew/bin/fish
  Binaries:
    Node: 20.11.0 - ~/.local/state/fnm_multishells/83610_1728461619670/bin/node
    Yarn: 4.1.0 - ~/.local/state/fnm_multishells/83610_1728461619670/bin/yarn
    npm: 10.2.4 - ~/.local/state/fnm_multishells/83610_1728461619670/bin/npm
  Browsers:
    Chrome: 129.0.6668.100
    Firefox: 130.0.1
    Safari: 18.0.1
  npmPackages:
    vitest: ^2.1.1 => 2.1.1

Used Package Manager

yarn

Validations

@jimmycallin jimmycallin changed the title Matchers doesn't work well with Proxy objects Matchers don't work well with Proxy objects Oct 9, 2024
@jimmycallin jimmycallin changed the title Matchers don't work well with Proxy objects toMatch doesn't work well with Proxy objects Oct 9, 2024
@jimmycallin jimmycallin changed the title toMatch doesn't work well with Proxy objects toMatchObject doesn't work well with Proxy objects Oct 9, 2024
@hi-ogawa
Copy link
Contributor

hi-ogawa commented Oct 10, 2024

Probably this is because toMatchObject is checking hasOwnProperty

Object.prototype.hasOwnProperty.call(object, key)

It looks like proxy can intercept more things. For example, something like this can make "more equivalent" object as plain one and toMatchObject and error diff work https://stackblitz.com/edit/vitest-dev-vitest-kuacmv?file=test%2Frepro.test.ts

example
const proxyObj = new Proxy(
  {},
  {
    get(target, prop, receiver) {
      if (prop === 'bar') {
        return 'foo';
      }
    },
    getOwnPropertyDescriptor(target, prop) {
      if (prop === 'bar') {
        return { configurable: true, enumerable: true };
      }
      return Reflect.getOwnPropertyDescriptor(target, prop);
    },
    ownKeys() {
      return ['bar'];
    },
  }
);

// pass
expect(proxyObj).toMatchObject({ bar: 'foo' })

// error diff
//   Object {
// -   "bar": "not-foo",
// +   "bar": "foo",
//   }
expect(proxyObj).toMatchObject({ bar: 'not-foo' })

Though this seems technically possible, I don' think people usually go this far for proxy implementation, so it might make sense to loosen toMatchObject (or specifically subsetEquality) to skip hasOwnProperty check. It looks mostly harmless removing this line though I haven't tested.

const result
= object != null
&& hasPropertyInObject(object, key)
&& equals(object[key], subset[key], [
...filteredCustomTesters,
subsetEqualityWithContext(seenReferences),
])

@hi-ogawa hi-ogawa added p2-to-be-discussed Enhancement under consideration (priority) and removed p2-to-be-discussed Enhancement under consideration (priority) labels Oct 10, 2024
@hi-ogawa
Copy link
Contributor

expect.objectContaining({ bar: "foo" }) also requires hasOwnProperty. So, if we change toMatchObject, then we probably should change this too.
https://stackblitz.com/edit/vitest-dev-vitest-dr5jfq?file=test%2Frepro.test.ts

for (const property in this.sample) {
if (
!this.hasProperty(other, property)
|| !equals(
this.sample[property],
other[property],
matcherContext.customTesters,
)
) {

@hi-ogawa hi-ogawa added p2-to-be-discussed Enhancement under consideration (priority) and removed pending triage labels Oct 10, 2024
@hi-ogawa hi-ogawa moved this to P2 - 3 in Team Board Oct 10, 2024
@sheremet-va sheremet-va moved this from P2 - 3 to Discussing in Team Board Oct 10, 2024
@hi-ogawa
Copy link
Contributor

@jimmycallin Btw, can you explain your actual use case? We think either behavior seems fine (current one or #6675), but knowing a concrete use case would help decisions. For example, is proxy coming from some well-known library or your specific implementation?

@jimmycallin
Copy link
Author

jimmycallin commented Oct 10, 2024

@jimmycallin Btw, can you explain your actual use case? We think either behavior seems fine (current one or #6675), but knowing a concrete use case would help decisions. For example, is proxy coming from some well-known library or your specific implementation?

Sure! It may be a bit esoteric, but we are using react-query which has a concept of tracked queries, which checks which properties you actually use to reduce rerenders in react. We have a wrapper around a useQuery function that adds a few properties, which naively would look something like this:

function useWrapper() {
    const query = useQuery(...);
    return {...query, additionalProperty: true }
}

Unfortunately the rest spread causes it to "trigger" each property, which nullifies the optimization. So we use a proxy to get around it:

function useWrapper() {
  const query = useQuery({...});
  return new Proxy(query, {
    get(target, prop, receiver) {
      if (prop === "additionalProp") {
        return true;
      }
      return Reflect.get(target, prop, receiver);
    },
  });
}

Then we use react-testing-library to test the result of the hook.

Hope this makes sense!

@hi-ogawa
Copy link
Contributor

Interesting, wrapping useQuery sounds like a sensible use case.
How about defining getOwnPropertyDescriptor / ownKeys? I guess it would be too tedious? (honestly I didn't even know such proxy handler exists 😄)

@jimmycallin
Copy link
Author

jimmycallin commented Oct 10, 2024

Interesting, wrapping useQuery sounds like a sensible use case. How about defining getOwnPropertyDescriptor / ownKeys? I guess it would be too tedious? (honestly I didn't even know such proxy handler exists 😄)

We had that previously, but just changed to Proxy after I saw this discussion that Proxy is a lot more performant: TanStack/query#8091

Which is what triggered this issue. :)

@hi-ogawa
Copy link
Contributor

hi-ogawa commented Oct 10, 2024

What I was suggesting is to implement getOwnPropertyDescriptor and ownKeys of proxy:

function useWrapper() {
  const query = useQuery({...});
  return new Proxy(query, {
    get(target, prop, receiver) {
      if (prop === "additionalProp") {
        return true;
      }
      return Reflect.get(target, prop, receiver);
    },
    getOwnPropertyDescriptor(target, prop) {
      if (prop === 'additionalProp') {
        return { configurable: true, enumerable: true };
      }
      return Reflect.getOwnPropertyDescriptor(target, prop);
    },
    ownKeys(target) {
      return [...Reflect.ownKeys(target, prop), 'additionalProp']
    },
  });
}

But, I think TanStack/query#8092 proves the point. If useQuery implements only Proxy.get, then Reflect.getOwnPropertyDescriptor won't probably work for downstream proxy.

EDIT: Oh wait, maybe their useQuery is fine because they have new Proxy(result, { get }), so default getOwnPropertyDescriptor/ownKeys can enumerate the ones from result.

@sheremet-va sheremet-va moved this from Discussing to Rejected in Team Board Dec 5, 2024
@hi-ogawa
Copy link
Contributor

hi-ogawa commented Dec 6, 2024

After discussing with the team, we decided to not remove hasOwnProperty check on our side.
Since proxy can be implemented properly to imitate an object, it's not good for test framework to go loose about this.

Here is an alternative if users need to implement proxy for Vitest assertions to work.

const obj = { foo: "bar", bar: "foo" };

const proxyBad = new Proxy(
  { foo: "bar" },
  {
    get(target, prop, receiver) {
      if (prop === "bar") {
        return "foo";
      }
    },
  },
);

const proxyGood1 = new Proxy(
  // 👇 define key on base object
  { foo: "bar", bar: undefined }, 
  {
    get(target, prop, receiver) {
      if (prop === "bar") {
        return "foo";
      }
    },
  },
);

const proxyGood2 = new Proxy(
  { foo: "bar" },
  {
    get(target, prop, receiver) {
      if (prop === "bar") {
        return "foo";
      }
      return Reflect.get(target, prop, receiver)
    },
    // 👇 define custom handler
    getOwnPropertyDescriptor(target, prop) {
      if (prop === 'bar') {
        return { configurable: true, enumerable: true };
      }
      return Reflect.getOwnPropertyDescriptor(target, prop);
    }
  },
);

Object.hasOwn(obj, 'bar') // true
Object.hasOwn(proxyBad, 'bar') // false
Object.hasOwn(proxyGood1, 'bar') // true
Object.hasOwn(proxyGood2, 'bar') // true

@hi-ogawa hi-ogawa closed this as completed Dec 6, 2024
@github-actions github-actions bot locked and limited conversation to collaborators Dec 21, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
p2-to-be-discussed Enhancement under consideration (priority)
Projects
Archived in project
Development

Successfully merging a pull request may close this issue.

2 participants