url: improve isURLThis detection

[anonrig]

Resolve the TODO and updates the implementation to avoid prototype look.

cc @TimothyGu

[nodejs-github-bot]

Review requested:

• @nodejs/url

[lemire]

@anonrig Is that faster?

[anonrig]

@anonrig Is that faster?

I don't think so. This was a recommendation by @TimothyGu on the original Ada migration pull request. I'm following up with the TODOs

[nodejs-github-bot]

CI: https://ci.nodejs.org/job/node-test-pull-request/50061/

[joyeecheung]

I wonder if we are just going to do a brand check, why can't we just make context a private field and let V8 perform the checks which is likely to be more performant...(that might be a breaking change, though I doubt if there's anyone relying on the error code being ERR_INVALID_THIS, the spec only says it wants a TypeError in those cases)

[anonrig]

I wonder if we are just going to do a brand check, why can't we just make context a private field and let V8 perform the checks which is likely to be more performant...(that might be a breaking change, though I doubt if there's anyone relying on the error code being ERR_INVALID_THIS, the spec only says it wants a TypeError in those cases)

I didn't follow. Can you share an example?

[aduh95]

@anonrig

class URL {
  #context;

  get href() {
    this.#context;
  }
}

Object.getOwnPropertyDescriptor(URL.prototype, 'href').get.call('not a URL object'); // TypeError

[anonrig]

I'll follow up on a different pull request since it might be a breaking change.

[nodejs-github-bot]

Landed in 37c736f

[victordidenko]

Oh.

In the original comment from @TimothyGu there was

e.g., Object.create(new URL(…)) should not be considered a URL

Can you please elaborate, why that so?
I think this is quite breaking change, that I cannot no more put URL as prototype... Actually this become unusable at all.

> Object.create(new URL('http://test.com'))
Uncaught TypeError [ERR_INVALID_THIS]: Value of "this" must be of type URL
    at __node_internal_captureLargerStackTrace (node:internal/errors:490:5)
    at new NodeError (node:internal/errors:399:5)
    at get href [as href] (node:internal/url:624:13)
    at [nodejs.util.inspect.custom] (node:internal/url:576:21)
    at formatValue (node:internal/util/inspect:806:19)
    at inspect (node:internal/util/inspect:365:10)
    at REPLServer.writer (node:repl:224:25)
    at finish (node:repl:945:32)
    at finishExecution (node:repl:550:7)
    at REPLServer.defaultEval (node:repl:639:7)
    at bound (node:domain:433:15)
    at REPLServer.runBound [as eval] (node:domain:444:12)
    at REPLServer.onLine (node:repl:902:10)
    at REPLServer.emit (node:events:525:35)
    at REPLServer.emit (node:domain:489:12)
    at [_onLine] [as _onLine] (node:internal/readline/interface:422:12)
    at [_line] [as _line] (node:internal/readline/interface:893:18)
    at [_ttyWrite] [as _ttyWrite] (node:internal/readline/interface:1271:22)
    at REPLServer.self._ttyWrite (node:repl:997:9)
    at ReadStream.onkeypress (node:internal/readline/interface:270:20)
    at ReadStream.emit (node:events:513:28)
    at ReadStream.emit (node:domain:489:12)
    at emitKeys (node:internal/readline/utils:357:14)
    at emitKeys.next (<anonymous>)
    at ReadStream.onData (node:internal/readline/emitKeypressEvents:64:36)
    at ReadStream.emit (node:events:513:28)
    at ReadStream.emit (node:domain:489:12)
    at addChunk (node:internal/streams/readable:324:12)
    at readableAddChunk (node:internal/streams/readable:297:9)
    at Readable.push (node:internal/streams/readable:234:10)
    at TTY.onStreamRead (node:internal/stream_base_commons:190:23)
    at TTY.callbackTrampoline (node:internal/async_hooks:130:17) {
  code: 'ERR_INVALID_THIS'

[TimothyGu]

@victordidenko If you try running Object.create(new URL('http://test.com')).href in the browser, you'll find that it doesn't work either. In short, a new object, whose prototype is a URL, should not be a URL in itself.

[victordidenko]

@TimothyGu Thank you! Yes, I know that already :)
I was hoping to see reference to exact place in specification though, because I cannot find it :( And I cannot understand why this object should break language fundamentals...
And I also wanted to emphasis that this is rather breaking change, and should not have been done in minor update, in my opinion. But also I'm not sure if node follows semver or not.

[lemire]

In the safari console (macOS):

[lemire]

In deno...

In bun...

[anonrig]

I think I see what the issue is... Object.create(new URL('https://yagiz.co')) worked in Node 16...

Welcome to Node.js v16.20.1.
Type ".help" for more information.
> Object.create(new URL('https://www.google.com'))
URL {
  href: 'https://www.google.com/',
  origin: 'https://www.google.com',
  protocol: 'https:',
  username: '',
  password: '',
  host: 'www.google.com',
  hostname: 'www.google.com',
  port: '',
  pathname: '/',
  search: '',
  searchParams: URLSearchParams {},
  hash: ''
}

[victordidenko]

It worked in 18.15

but in 18.16 this fix has landed

[victordidenko]

I totally agree with that URL should behave the same in all environments. I was just using this feature in my tests, and was really surprised, that node update from 18.15 to 18.16 broke my tests. But I've already rewrote them, and for now I'm just curious, why this behaviour in the first place?