fix(assert): support Request objects in equal()

[iuioiua]

Previously, this would evaluate to false because of internal symbols within Request objects being unequal:

import { equal } from "jsr:@std/assert/equal`;
equal(
  new Request("https://example.com"),
  new Request("https://example.com")
); // false

This would have knock-on effects due to equal() being the underlying function for comparison within assertEquals(), assertSpyCall(), etc.

This PR is a quick and dirty fix for handling the comparison of said Request objects in equal(). I'm sure there's a better way to do this, but at least this improves equal() a little.

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 94.26%. Comparing base (e80e6bb) to head (7487f77).
⚠️ Report is 2 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #6982      +/-   ##
==========================================
- Coverage   94.26%   94.26%   -0.01%     
==========================================
  Files         602      602              
  Lines       43662    43678      +16     
  Branches     7063     7071       +8     
==========================================
+ Hits        41159    41174      +15     
- Misses       2447     2448       +1     
  Partials       56       56              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[tomas-zijdemans]

I might be wrong, but I think input validation is the better way of handling this. We could add:

if (a instanceof Request) {
  throw new TypeError("Cannot compare Request instances");
}
if (a instanceof Response) {
  throw new TypeError("Cannot compare Response instances");
}

This should be added right after the WeakMap and WeakSet input validation check.

We could consider doing the same for functions like described here.

[lionel-rowe]

I might be wrong, but I think input validation is the better way of handling this.

This seems reasonable, given that Request objects (along with Response, Blob, File, etc.) can't be reliably compared synchrously unless they're empty. And in the case of Request and Response, even body: "" counts as non-empty for purposes of comparison.

With logic as of current PR:

// true
equal(
    new Request('null://', { method: 'POST', body: '' }),
    new Request('null://', { method: 'POST', body: 'some content' }),
)

[tomas-zijdemans]

Here's an alternative PR that solves it with input validation instead: #6984.

[kt3k]

This change feels confusing to me too as bodies are not compared as pointed by Lionel.

There is special note about Blob in doc of assertEquals. How about adding similar note about comparing Requests?

@iuioiua In what situation, did you find the existing behavior inconvenient? Can you share some examples?

[iuioiua]

This change feels confusing to me too as bodies are not compared as pointed by Lionel.

Again, while not true 100% of the time, it's likely that the bodies are the same if other properties are the same. E.g. if the content-length header, the content-type header and the bodyUsed properties are equal, it's more likely than not that the bodies are equal in most cases. I agree, simply adding an explanation or example to the related functions in how to deal Request objects (clone requests and compare their bodies separately).

@iuioiua In what situation, did you find the existing behavior inconvenient? Can you share some examples?

import { assertSpyCall, stub } from "@std/testing/mock";

using fetchStub = stub(globalThis, "fetch");
await fetch(new Request("https://example.com"));

assertSpyCall(fetchStub, 0, {
  args: [new Request("https://example.com")],
});

In my specific use case, the package I'm using passes in a Request object to the 1st argument of fetch(), which is a valid way to use the Fetch API.

[lionel-rowe]

@iuioiua content-length would in theory be the highest volatility and thus most likely to prevent false positives. It still wouldn't eliminate them entirely, but more importantly, it isn't visible to JS in the first place:

new Request('foo:', { method: 'POST', body: 'bar' }).headers.get('content-length') // null

Checking body for reference equality could work, which would at least enable use cases such as your example in which no body is set. Thus for two non-reference-equal requests, body: null == body: null but body: '' != body: '' (because the latter is an empty ReadableStream when read).

[iuioiua]

Yes, of course the content-length header can be incorrect. As I already said, this is not full proof. But it beats being completely unable to check that 2 of these objects aren't equal in practical terms.

[lionel-rowe]

Yes, of course the content-length header can be incorrect.

It's less that it's incorrect, more that it's usually null when read from JS. I think in practice this will depend on whether it's user-constructed or received via HTTP, and if the latter, whether it's exposed via CORS, and also whether a request uses chunked encoding or HTTP 2, neither of which set it.

As I already said, this is not full proof. But it beats being completely unable to check that 2 of these objects aren't equal in practical terms.

IMO a function that promises to check for equality should at minimum never return false positives, except maybe in extreme corner cases. False negatives, while best avoided where possible, are more forgivable when those are based on properties that aren't reference equal and that can't be checked for value equality. In some such cases, such as the case of contentful Requests, you could even argue that those aren't false negatives at all (because from the standpoint of synchronous code, two different ReadableStreams can never be assumed equal).

[tomas-zijdemans]

I propose to close this PR, since we went with the documentation based approach: #6989