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

[Bug]: structuredClone fails on toStrictEqual #14011

Open
JamieMagee opened this issue Mar 16, 2023 · 44 comments
Open

[Bug]: structuredClone fails on toStrictEqual #14011

JamieMagee opened this issue Mar 16, 2023 · 44 comments

Comments

@JamieMagee
Copy link

JamieMagee commented Mar 16, 2023

Version

29.4.0

Steps to reproduce

  1. Clone my repository at https://github.com/JamieMagee/jest-structuredclone-strictequal/blob/master/index.spec.js
  2. npm install
  3. npm test

Expected behavior

I expect to see objects cloned with structuredClone to pass toStrictEqual

Actual behavior

The values do not pass toStrictEqual (but do pass toEqual).

jest › toStrictEqual › structured clone

    expect(received).toStrictEqual(expected) // deep equality

    Expected: {"value": "test"}
    Received: serializes to the same string

       6 |     describe('toStrictEqual', () => {
       7 |         it('structured clone', () => {
    >  8 |             expect(structuredClone(value)).toStrictEqual(value);
         |                                            ^
       9 |         });
      10 |         it('JSON clone', () => {
      11 |             expect(JSON.parse(JSON.stringify(value))).toStrictEqual(value);

      at Object.toStrictEqual (index.spec.js:8:44)

However JSON.parse(JSON.stringify()) does pass toStrictEqual

Additional context

The test output can be seen in this GitHub Actions run.

Environment

System:
  OS: Linux 6.2 NixOS 23.05 (Stoat) 23.05 (Stoat)
  CPU: (32) x64 AMD Ryzen 9 7950X 16-Core Processor
Binaries:
  Node: 19.7.0 - /run/current-system/sw/bin/node
  Yarn: 1.22.19 - /run/current-system/sw/bin/yarn
  npm: 9.5.0 - /run/current-system/sw/bin/npm
@JamieMagee JamieMagee changed the title [Bug]: structuredclone fails on toStrictEqual [Bug]: structuredClone fails on toStrictEqual Mar 16, 2023
@mrazauskas
Copy link
Contributor

I was playing with assert and expect:

import assert from "node:assert";
import { expect } from "@jest/globals";

// equality

assert.deepEqual(JSON.parse(JSON.stringify(value)), value); // pass
expect(JSON.parse(JSON.stringify(value))).toEqual(value); // pass

assert.deepEqual(structuredClone(value), value); // pass
expect(structuredClone(value)).toEqual(value); // pass

// strict equality

assert.deepStrictEqual(JSON.parse(JSON.stringify(value)), value); // pass
expect(JSON.parse(JSON.stringify(value))).toStrictEqual(value); // pass

assert.deepStrictEqual(structuredClone(value), value); // fail !!!
expect(structuredClone(value)).toStrictEqual(value); // fail !!!

Hm.. if Node and Expect agrees, that is correct behaviour. Or did I miss something?

@mrazauskas
Copy link
Contributor

mrazauskas commented Mar 17, 2023

Might be this is the explanation:

Object.getPrototypeOf(value) === Object.getPrototypeOf(JSON.parse(JSON.stringify(value))) // true

Object.getPrototypeOf(value) === Object.getPrototypeOf(structuredClone(value)) // false

@JamieMagee
Copy link
Author

Okay, it looks like this might be a limitation of structuredClone and we should use toEqual instead. From MDN (emphasis mine):

Certain object properties are not preserved:

  • The lastIndex property of RegExp objects is not preserved.
  • Property descriptors, setters, getters, and similar metadata-like features are not duplicated. For example, if an object is marked readonly with a property descriptor, it will be read/write in the duplicate, since that's the default.
  • The prototype chain is not walked or duplicated.

@thernstig
Copy link
Contributor

My gut feeling is that I would not want to stop using .toStrictEqual(value) but would prefer another solution than to start using .toEqual(value).

.toStrictEqual(value) verifies a lot of other nice things. From https://jestjs.io/docs/expect#tostrictequalvalue:

  • keys with undefined properties are checked, e.g. {a: undefined, b: 2} will not equal {b: 2};
  • undefined items are taken into account, e.g. [2] will not equal [2, undefined];
  • array sparseness is checked, e.g. [, 1] will not equal [undefined, 1];
  • object types are checked, e.g. a class instance with fields a and b will not equal a literal object with fields a and b.

I wish to not loose all these checks when using structuredClone().

I am not sure of the solution. One option could be to in a major version (i.e. a breaking change) omitting .toStrictEqual(value) to check for the prototype. Another option would be a new function.

Any other options we have?

@mrazauskas
Copy link
Contributor

Indeed it would be useful to be able to use .toStrictEqual() in this case as well. What about: .toStrictEqual(value, {skipPrototypeCheck: true}) or similar option?

@thernstig
Copy link
Contributor

thernstig commented Apr 1, 2023

@mrazauskas that is one solution. It kind of goes against other APIs at https://jestjs.io/docs/expect. Few function have a second argument. And if they do, it is not an object (possibly for readability?).

If a second argument is the way to go I think this is nicer: .toStrictEqual(value, skipPrototypeCheck?) . Could/would be nice to add to the docs for the argument skipPrototypeCheck that it can be used when calling code that uses structuredClone(). Just as a common example.

I am not sure if @SimenB has more input on how a new API to cover this should look. Argument, new function or something else.

@mrazauskas
Copy link
Contributor

mrazauskas commented Apr 1, 2023

Not sure how you define readability. A bag of options is easier to extend in the future and is understandable without looking at the docs:

.toStrictEqual(value, {skipPrototypeCheck: true, skipUndefinedCheck: false})
.toStrictEqual(value, true, false)

.toStrictEqual(value, {skipUndefinedCheck: false})
.toStrictEqual(value, undefined, false)

@thernstig
Copy link
Contributor

Yeah, I agree it is subjective.

Would a new toBeClone() add too many similar APIs?

@mrazauskas
Copy link
Contributor

Great idea! I could see .toBeClone() implemented in Jest Extended next to .toBeFrozen() and .toBeSealed().

I guess prototype should be somehow replaced for both objects before sending them into equals() and that is it. Or?

@thernstig
Copy link
Contributor

Selfish wish would be for it to be part of the core Jest expect functions, since we avoid Jest Extended as to not complicate users with too many options. But that is entirely subjective, as much is 😆 One reason to keep it on the core Jest Expect lib would be that structuredClone() is a standard/global API that exists on the global object.

As for the question about equals() I do not know the details how the comparison is done internally. In general, replacing the prototype is considered bad and not great for performance, see https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Object/setPrototypeOf. Not sure if that detail adds any value to how equals() works?

@mrazauskas
Copy link
Contributor

mrazauskas commented Apr 1, 2023

By the way, I was trying to implement a custom matcher and came to this idea:

const value = { test: 123 };
const received = structuredClone(value);

expect(received).toStrictEqual(structuredClone(value)); // pass !!!

@github-actions
Copy link

github-actions bot commented May 1, 2023

This issue is stale because it has been open 30 days with no activity. Remove stale label or comment or this will be closed in 30 days.

@github-actions github-actions bot added the Stale label May 1, 2023
@thernstig
Copy link
Contributor

I suppose this is still a great feature to have.

@github-actions github-actions bot removed the Stale label May 2, 2023
@Marchelune
Copy link

Hi there ! I just stumbled upon the same issue, and I agree that, given structuredClone is now part of the standard library and more and more codebases, there should be an appropriate test for it.

One particular sample that should pass IMO is the following:

describe('foo', () => {
    test('date check', () => {
        const date = new Date();
        expect(structuredClone(date)).toStrictEqual(date);
    });
});

This currently fails with


expect(received).toStrictEqual(expected) // deep equality

    Expected: 2023-05-23T13:12:11.576Z
    Received: serializes to the same string

This is key to me because using the JSON.parse(JSON.stringify(...)) technique does not accommodate nested dates objects (they are serialised to strings), where structuredClone does.
I don't really get what Jest sees to differentiate the original and the clone in the date exemple above, aren't the prototypes the same?

@thernstig
Copy link
Contributor

thernstig commented May 23, 2023

@Marchelune if you read this thread and #14011 (comment) and also the comment after it, it answers the question.

@Marchelune
Copy link

@thernstig I did read that and I saw the prototypes aren't the same but it still doesn't explain why it is so.

@thernstig
Copy link
Contributor

@thernstig it specifically says The prototype chain is not walked or duplicated. which means after structuredClone the new object has no object sets for its prototype, meaning it cannot be the same. Does that not explain it?

@github-actions
Copy link

This issue is stale because it has been open 30 days with no activity. Remove stale label or comment or this will be closed in 30 days.

@github-actions github-actions bot added the Stale label Jun 23, 2023
@thernstig
Copy link
Contributor

Unstale

@github-actions github-actions bot removed the Stale label Jun 26, 2023
@github-actions
Copy link

This issue is stale because it has been open 30 days with no activity. Remove stale label or comment or this will be closed in 30 days.

@github-actions github-actions bot added the Stale label Jul 26, 2023
@thernstig
Copy link
Contributor

Feels important enough to keep alive as I suspect modern JavaScript will use structuredClone

@github-actions github-actions bot added the Stale label Oct 15, 2023
@thernstig
Copy link
Contributor

Comment. Still a bug.

@github-actions github-actions bot removed the Stale label Oct 15, 2023
@LinusU
Copy link
Contributor

LinusU commented Oct 18, 2023

@thernstig it specifically says The prototype chain is not walked or duplicated. which means after structuredClone the new object has no object sets for its prototype, meaning it cannot be the same. Does that not explain it?

I don't think that's correct. My reading is that it won't recursively deep clone the prototype chain, which makes sense.

This can be seen by trying out these two lines in your browser or Node.js repl:

structuredClone([]).constructor === [].constructor
// => true

Object.getPrototypeOf([]) === Object.getPrototypeOf(structuredClone([]))
// => true

I think that this is Jest specific, because trying the same within the Jest runner results in another behaviour:

console.error(structuredClone([]).constructor === [].constructor)
  ● Console

    console.error
      false

    > 119 |       console.error(structuredClone([]).constructor === [].constructor)
          |               ^


My guess is that this has something to do with worker threads/sandboxing/custom global environment, where maybe the global Array isn't the same as the Array constructor that structuredClone uses. But I'm not familiar enough with the inner workings of Jest to say for sure...

@alecmev
Copy link

alecmev commented Oct 19, 2023

I'm a little confused about the recent discourse, doesn't the analysis I posted explain the bug? #14011 (comment) Now a collaborator just needs to take a look and decide what next.

Copy link

This issue is stale because it has been open 30 days with no activity. Remove stale label or comment or this will be closed in 30 days.

@github-actions github-actions bot added the Stale label Nov 18, 2023
@thernstig
Copy link
Contributor

Be kind to us bot

@github-actions github-actions bot removed the Stale label Nov 19, 2023
Copy link

This issue is stale because it has been open 30 days with no activity. Remove stale label or comment or this will be closed in 30 days.

@github-actions github-actions bot added the Stale label Dec 19, 2023
@thernstig
Copy link
Contributor

Be kind to us bot

@github-actions github-actions bot removed the Stale label Dec 19, 2023
Copy link

This issue is stale because it has been open 30 days with no activity. Remove stale label or comment or this will be closed in 30 days.

@github-actions github-actions bot added the Stale label Jan 18, 2024
@LinusU
Copy link
Contributor

LinusU commented Jan 23, 2024

pls no stale

@github-actions github-actions bot removed the Stale label Jan 23, 2024
Copy link

This issue is stale because it has been open 30 days with no activity. Remove stale label or comment or this will be closed in 30 days.

@github-actions github-actions bot added the Stale label Feb 22, 2024
@Marchelune
Copy link

this is still open IMO.

Copy link

This issue is stale because it has been open 30 days with no activity. Remove stale label or comment or this will be closed in 30 days.

@github-actions github-actions bot added the Stale label Mar 25, 2024
@Marchelune
Copy link

This is still relevant.

@github-actions github-actions bot removed the Stale label Mar 26, 2024
Copy link

This issue is stale because it has been open 30 days with no activity. Remove stale label or comment or this will be closed in 30 days.

@github-actions github-actions bot added the Stale label Apr 25, 2024
@si14
Copy link

si14 commented Apr 25, 2024

Not stale

@github-actions github-actions bot removed the Stale label Apr 25, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

8 participants