Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Cannot reliably compare Ada-based URLs (Node.js v18.17.0 & newer) #97

Open
markandrus opened this issue Jul 19, 2023 · 2 comments
Open

Comments

@markandrus
Copy link

Until recently, my team was using Node.js v18.13.0. Our CI system picked up Node.js v18.17.0 and started failing. Specifically, we started failing when using Chai to perform expect(…).to.deep.equal on URLs.

After reading the changelog for v18.17.0, I see that Ada-based URLs were backported to Node.js 18 (see here).

Narrowing down some more, it seems there is a behavior change in Node.js 18.17.0 where Symbol(query) is lazily set on URLs whenever searchParams is accessed (see here). So, if you've never accessed that property of a URL, Symbol(query) does not exist; however, once you have accessed that property, Symbol(query) does exist. This leads to the following breakage (I'm demonstrating with Mocha):

const { expect } = require('chai')

describe('URL', () => {
  it('succeeds', () => {
    const url = new URL('foo://bar')
    expect(url).to.deep.equal(new URL('foo://bar'))
  })

  // The following succeeded in Node.js < v18.17.0.
  it('fails', () => {
    const url = new URL('foo://bar')
    void url.searchParams
    expect(url).to.deep.equal(new URL('foo://bar'))
  })
})

Maybe this is working as expected, but it was an unfortunate bug we hit. We have worked around it by changing our tests to no longer expect(…).to.deep.equal on URLs.

@keithamus
Copy link
Member

keithamus commented Jul 19, 2023

Is searchParams enumerable? I think a URL will land here:

function keysEqual(leftHandOperand, rightHandOperand, keys, options) {
and each key will be checked by getting the property, so I don't know why it would fail.

Oh... unless you're saying the property that houses Symbol(query) is enumerable?

@markandrus
Copy link
Author

Hey @keithamus,

Oh... unless you're saying the property that houses Symbol(query) is enumerable?

Yes! Symbol(query) is enumerable, and in Node.js < v18.17.0, Symbol(query) it was always present. Now, in Node.js v18.17.0 (or any version using Ada), Symbol(query) is lazily computed once searchParams is accessed. We can see that here:

Node.js v18.17.0

$ node
Welcome to Node.js v18.17.0.
Type ".help" for more information.
> var url = new URL('foo://bar')
undefined
> Object.getOwnPropertyNames(url)
[]
> Object.getOwnPropertySymbols(url)
[ Symbol(context) ] // ← Symbol(query) is missing
> void url.searchParams
undefined
> Object.getOwnPropertyNames(url)
[]
> Object.getOwnPropertySymbols(url)
[ Symbol(context), Symbol(query) ] // ← Symbol(query) is present and enumerable after accessing searchParams

Node.js v18.13.0

$ node                       
Welcome to Node.js v18.13.0.
Type ".help" for more information.
> var url = new URL('foo://bar')
undefined
> Object.getOwnPropertyNames(url)
[]
> Object.getOwnPropertySymbols(url)
[ Symbol(context), Symbol(query) ] // ← Symbol(query) is present and enumerable
> void url.searchParams
undefined
> Object.getOwnPropertyNames(url)
[]
> Object.getOwnPropertySymbols(url)
[ Symbol(context), Symbol(query) ] // ← Symbol(query) is still present and enumerable

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

No branches or pull requests

2 participants