[v18.17] Same URLs are different

[regseb]

Version

v18.17.0

Platform

Linux regseblaptop 5.19.0-46-generic #47~22.04.1-Ubuntu SMP PREEMPT_DYNAMIC Wed Jun 21 15:35:31 UTC 2 x86_64 x86_64 x86_64 GNU/Linux

Subsystem

No response

What steps will reproduce the bug?

import assert from "node:assert/strict";

const link = "https://baz.org/";
const url = new URL(link);

console.log("===== FIRST =====")
assert.deepEqual(url, new URL(link));

console.log("===== CONSOLE =====")
console.log(url);

console.log("===== SECOND =====")
assert.deepEqual(url, new URL(link));

How often does it reproduce? Is there a required condition?

The error occurs since Node.js v18.17.0.

What is the expected behavior? Why is that the expected behavior?

===== FIRST =====
===== CONSOLE =====
URL {
  href: 'https://baz.org/',
  origin: 'https://baz.org',
  protocol: 'https:',
  username: '',
  password: '',
  host: 'baz.org',
  hostname: 'baz.org',
  port: '',
  pathname: '/',
  search: '',
  searchParams: URLSearchParams {},
  hash: ''
}
===== SECOND =====

What do you see instead?

===== FIRST =====
===== CONSOLE =====
URL {
  href: 'https://baz.org/',
  origin: 'https://baz.org',
  protocol: 'https:',
  username: '',
  password: '',
  host: 'baz.org',
  hostname: 'baz.org',
  port: '',
  pathname: '/',
  search: '',
  searchParams: URLSearchParams {},
  hash: ''
}
===== SECOND =====
node:internal/process/esm_loader:97
    internalBinding('errors').triggerUncaughtException(
                              ^

AssertionError [ERR_ASSERTION]: Expected values to be strictly deep-equal:
+ actual - expected ... Lines skipped

+ <ref *1> URL {
- URL {
    [Symbol(context)]: URLContext {
      hash_start: 4294967295,
...
      search_start: 4294967295,
      username_end: 8
+   },
+   [Symbol(query)]: URLSearchParams {
+     [Symbol(context)]: [Circular *1],
+     [Symbol(query)]: []
    }
  }
    at file:///home/regseb/testcase/index.js:13:8
    at ModuleJob.run (node:internal/modules/esm/module_job:194:25) {
  generatedMessage: true,
  code: 'ERR_ASSERTION',
  actual: <ref *1> URL {
    [Symbol(context)]: URLContext {
      href: 'https://baz.org/',
      protocol_end: 6,
      username_end: 8,
      host_start: 8,
      host_end: 15,
      pathname_start: 15,
      search_start: 4294967295,
      hash_start: 4294967295,
      port: 4294967295,
      scheme_type: 2
    },
    [Symbol(query)]: URLSearchParams {
      [Symbol(query)]: [],
      [Symbol(context)]: [Circular *1]
    }
  },
  expected: URL {
    [Symbol(context)]: URLContext {
      href: 'https://baz.org/',
      protocol_end: 6,
      username_end: 8,
      host_start: 8,
      host_end: 15,
      pathname_start: 15,
      search_start: 4294967295,
      hash_start: 4294967295,
      port: 4294967295,
      scheme_type: 2
    }
  },
  operator: 'deepStrictEqual'
}

Node.js v18.17.0

Additional information

• Node.js v18.17.0 comes with the latest version of the URL parser, Ada.
• The difference is found only after console.log().

[debadree25]

cc @nodejs/url

[anonrig]

Calling console.log lazy loads the query parameter and initializes the searchParams causing this indifference when using deepEqual. Unfortunately, I don’t have a solution to this problem without seriously impacting the performance.

[regseb]

I tested with Node.js v20.5.0 and I have no problem.

const url = new URL("http://foo.org");

console.log(Object.getOwnPropertySymbols(url));
url.searchParams;
console.log(Object.getOwnPropertySymbols(url));

Node.js v18.17.0	[ Symbol(context) ] [ Symbol(context), Symbol(query) ]
Node.js v20.5.0	[] []
Chromium 114	[] []

@anonrig Will this problem happen on v20? Or will it only be on v18?

[anonrig]

We use private properties in Node 20 because it is a breaking change coming from using symbols as a private property. That’s why the output is different in Node 18.

[anonrig]

By the way, if you want to do a equality check between url instances, compare their href attributes. That would be the fastest and most direct way.

[aduh95]

Unfortunately, I don’t have a solution to this problem without seriously impacting the performance.

Do you mean using a WeakMap? How bad are we talking about?

[anonrig]

Unfortunately, I don’t have a solution to this problem without seriously impacting the performance.

Do you mean using a WeakMap? How bad are we talking about?

The root cause of this inequality causes from console.log(url) where it calls searchParams getter (ref: https://github.com/nodejs/node/blob/main/lib/internal/url.js#L809). When searchParams getter is run, if it's not initialized, it initializes URLSearchParams and assigns that variable. But if you have 2 different URL's, in which URLSearchParams is initialized, and in another, it is not, this inequality occurs.

Available solutions:

• not trigger searchParams getter on inspect, with the loss of context in inspect function.
• remove the lazy approach of URLSearchParams on searchParams getter, which would impact the execution time of new URL() since it would also call URLSearchParams constructor.

[aduh95]

Wouldn't another solution to have a WeakMap to store the query rather than adding a Symbol to the URL instance? Instead of having something like

const searchParams = Symbol('query');

class URL{
  get searchParams() {
    if (!isURL(this))
      throw new ERR_INVALID_THIS('URL');
    if (this[searchParams] == null) {
      this[searchParams] = new URLSearchParams(this.search);
      this[searchParams][context] = this;
    }
    return this[searchParams];
  }
}

we could have something like:

const searchParams = new SafeWeakMap();
class URL{
  get searchParams() {
    if (!isURL(this))
      throw new ERR_INVALID_THIS('URL');
    const cachedValue = searchParams.get(this)
    if (cachedValue != null) {
      return cachedValue;
    }
    const value = new URLSearchParams(this.search);
    value[context] = this;
    searchParams.set(this, value);
    return value;
  }
}

It'd still be lazy but wouldn't affect the deep equality.

[anonrig]

Might work. Can you open a pull request to v18.x-staging?

[markandrus]

Fixing this should also fix Chai: chaijs/deep-eql#97

[anonrig]

The latest v18.18 fixes this issue.