Skip to content

Add deepStrictEqual support - #6

Merged
yassernasc merged 27 commits into
mainfrom
deepStrictEqual
Aug 17, 2026
Merged

Add deepStrictEqual support#6
yassernasc merged 27 commits into
mainfrom
deepStrictEqual

Conversation

@yassernasc

@yassernasc yassernasc commented Aug 3, 2026

Copy link
Copy Markdown
Contributor

@yassernasc
yassernasc requested a review from a team August 3, 2026 21:49
@yassernasc
yassernasc marked this pull request as draft August 3, 2026 21:49
@yassernasc
yassernasc marked this pull request as ready for review August 4, 2026 21:22

@kasperisager kasperisager left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This only scratches the surface of deep equality testing, see https://github.com/nodejs/node/blob/main/lib/internal/util/comparisons.js for the corresponding implementation in Node.js. We don't need to match that 1:1, but we should cover as much as is reasonable.

Comment thread lib/memoize-map.js Outdated
Comment thread lib/memoize-map.js Outdated
Comment thread lib/memoize-map.js Outdated
Comment thread lib/memoize-map.js Outdated
Comment thread index.js Outdated
Comment thread index.js Outdated
Comment thread index.js Outdated
Comment thread index.js Outdated
Comment thread index.js Outdated
@yassernasc
yassernasc marked this pull request as draft August 5, 2026 13:17
@yassernasc

yassernasc commented Aug 5, 2026

Copy link
Copy Markdown
Contributor Author

I'll double check a few more cases to cover; Currently, circular structures have false-positives for cases such as: https://github.com/nodejs/node/blob/main/test/parallel/test-assert-deep.js#L598-L607.

Edit: I let two unit tests commented, those are limitations for the simplicity of the current memoization solution.

@yassernasc
yassernasc requested a review from kasperisager August 6, 2026 18:25
@yassernasc
yassernasc marked this pull request as ready for review August 7, 2026 12:50
Comment thread index.js Outdated
Comment on lines +134 to +142
if (
prototype === BigInt.prototype ||
prototype === Boolean.prototype ||
prototype === Number.prototype ||
prototype === String.prototype ||
prototype === Symbol.prototype
) {
return deepStrictEqualValue(a.valueOf(), b.valueOf(), memo)
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Testing literal prototype identities won't work for cross-realm checks so we'll need additional libjs APIs for this, I'll add those. It's good enough for the initial implementation though 👌

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

holepunchto/libjs#51, we can handle that in a separate PR once the new APIs are out.

@kasperisager kasperisager left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've added some additional test cases that must be covered as well to bring us mostly in line with the Node.js implementation.

@yassernasc

Copy link
Copy Markdown
Contributor Author

I named the new utility class Node to link the idea of an algorithm for graphs.

@kasperisager kasperisager left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Solid progress! I added some more tests.

@yassernasc

yassernasc commented Aug 11, 2026

Copy link
Copy Markdown
Contributor Author

The comment "what matters is how many objects are reachable from the root before one repeats" gave me the hint of a possibility to simplify the strategy of memoization, and now, the comparison for cycles relies on the length of the circular objects.

The allowDuplicates flag wasn't planned at first, but it's necessary because of the quadratic loop from "deepStrictEqualArrayUnordered", as it may pass the same values multiple times to "deepStrictEqualValue" while performing a single (a, b) test.

@kasperisager kasperisager left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Getting closer! I've added some more tests.

@yassernasc
yassernasc marked this pull request as draft August 13, 2026 14:28
@yassernasc
yassernasc marked this pull request as ready for review August 14, 2026 14:34
kasperisager
kasperisager previously approved these changes Aug 15, 2026
@yassernasc
yassernasc merged commit 9bbd7e1 into main Aug 17, 2026
4 checks passed
@yassernasc
yassernasc deleted the deepStrictEqual branch August 17, 2026 11:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants