assertEquals fails to throw on unequal instances of built-ins such as Temporal APIs (with --unstable-temporal flag) or URLSearchParams

[lionel-rowe]

Describe the bug

assertEquals fails when comparing various Temporal instances with polyfill or --unstable-temporal flag. It also similarly fails to compare other built-ins, such as URLPattern and URLSearchParams.

Steps to Reproduce

deno repl --unstable-temporal

import { assert, assertEquals, assertThrows } from 'jsr:@std/[email protected]'

// doesn't throw
assertEquals(
    Temporal.PlainDate.from('1999-09-09'),
    Temporal.PlainDate.from('1212-12-12'),
)

// throws in Deno (see https://github.com/denoland/std/issues/6151#issuecomment-2440809083); doesn't throw in browser
assertEquals(
    new URLSearchParams('?a=1&b=2'),
    new URLSearchParams('?a=1&b=99999'),
)

Equivalent results can be observed with unequal instances of Temporal.Instant, Temporal.ZonedDateTime, URLPattern, etc.

Expected behavior

assertEquals to throw when the instances are not value-equal.

Environment

OS: Ubuntu 20.04, WSL on Windows 11
deno version: 2.0.3
std version: [email protected]

[lionel-rowe]

It looks like it's because objects are compared using own-properties, but all the properties of Temporal instances are defined on the prototype instead. I suspect this also means similar bugs exist for many other built-in and user-defined classes.

Possible means of patching for Temporal could include comparing toString or valueOf; a more generic fix could include comparing non-own properties up the prototype chain (not sure about perf implications though).

[kt3k]

Possible means of patching for Temporal could include comparing toString or valueOf;

Adding special path for builtin Temporal makes sense to me (We already have special handling for Date). I don't think we need to do special things about polyfills.

[lionel-rowe]

Adding special path for builtin Temporal makes sense to me (We already have special handling for Date). I don't think we need to do special things about polyfills.

I agree that there should be no special handling for polyfills, but also any solution for built-in Temporal should work seamlessly for any correctly implemented polyfill. I only mentioned that the issue also affects the polyfill to rule out the possibility it could be a bug in the V8 harmony Temporal implementation.

I'm not sure to what extent special-casing Temporal really makes sense, except maybe implementing a fast path if that noticeably improves perf. Notably, URL and RegExp are similarly special-cased as well as Date, but other built-ins that library consumers might reasonably want to compare, such as URLSearchParams and URLPattern, are not.

Upon further testing, URLSearchParams and URLPattern also fail to throw when unequal do work correctly within Deno, but it seems that's only thanks to Deno's non-standard addition of own-property symbols to instances of those objects (e.g. URLPattern having a Symbol("components") property). So such comparisons work correctly within Deno, but fail in the browser. I'm not sure what the rationale was for adding those symbol properties in the first place, but it might be worth revisiting their use (e.g. switching to WeakMaps) given that it introduces inconsistency between Deno and browsers. Looks like there's an issue for this already: denoland/deno#11720

[kt3k]

Upon further testing, URLSearchParams and URLPattern also fail to throw when unequal do work correctly within Deno, but it seems that's only thanks to Deno's non-standard addition of own-property symbols to instances of those objects (e.g. URLPattern having a Symbol("components") property). So such comparisons work correctly within Deno, but fail in the browser. I'm not sure what the rationale was for adding those symbol properties in the first place, but it might be worth revisiting their use (e.g. switching to WeakMaps) given that it introduces inconsistency between Deno and browsers. Looks like there's an issue for this already: denoland/deno#11720

Fair enough. Dependency to Deno's specific implementation doesn't sound great. #6153 makes sense to me.