Skip to content

fix(ext/node): deep assert compatibility#32434

Draft
Tango992 wants to merge 8 commits intodenoland:mainfrom
Tango992:node-assert-deep-equal
Draft

fix(ext/node): deep assert compatibility#32434
Tango992 wants to merge 8 commits intodenoland:mainfrom
Tango992:node-assert-deep-equal

Conversation

@Tango992
Copy link
Member

@Tango992 Tango992 commented Mar 3, 2026

Allows https://github.com/nodejs/node/blob/v24.12.0/test/parallel/test-assert-deep.js to pass. This marks 100% coverage of node compat tests related to the assert module.

@Tango992 Tango992 added the ci-draft Run the CI on draft PRs. label Mar 4, 2026
Copy link
Contributor

@kajukitli kajukitli left a comment

Choose a reason for hiding this comment

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

looks good overall - nice to get 100% assert coverage.

couple small things to note (not blockers):

  1. tc_scope rethrow pattern in util.rs - the pattern is sound, but the dummy array allocation after rethrow feels weird. might be worth a brief comment like "v8 will discard this return value" or something. minor readability thing.

  2. kKeyObject on CryptoKey - clever solution for making WebCrypto keys comparable with Node's KeyObjects. makes sense.

  3. isError refactor - good cleanup, consolidating the isNativeError || ObjectPrototypeIsPrototypeOf(ErrorPrototype, ...) pattern into a single helper.

  4. console isURL check - the check does typeof value.href === 'string' before the prototype check which is good for perf (avoids slower prototype lookup on non-URL objects).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

ci-draft Run the CI on draft PRs.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants