module: use isURLInstance instead of instanceof

[aduh95]

Related to #34622, in case there are several WHATWG URL implementations available, isURLInstance would work better than instanceof URL. Might also be more efficient, but I haven't run any benchmarks :)

Checklist

• make -j4 test (UNIX), or vcbuild test (Windows) passes
• commit message follows commit guidelines

[nodejs-github-bot]

Review requested:

• @nodejs/modules

[ljharb]

this is a good change - instanceof should be avoided - but perhaps this should have a test using the vm module?

[aduh95]

perhaps this should have a test using the vm module?

@ljharb can you elaborate on what you have in mind? URL is not a primordial object, it's not available to VM Script instances:

$ node -p "require('vm').runInContext('URL', require('vm').createContext({})) === URL"
evalmachine.<anonymous>:1
URL
^

ReferenceError: URL is not defined
    at evalmachine.<anonymous>:1:1
    at Script.runInContext (vm.js:141:18)
    at Object.runInContext (vm.js:279:6)
    at [eval]:1:15
    at Script.runInThisContext (vm.js:131:18)
    at Object.runInThisContext (vm.js:295:38)
    at Object.<anonymous> ([eval]-wrapper:10:26)
    at Module._compile (internal/modules/cjs/loader.js:1200:30)
    at evalScript (internal/process/execution.js:98:25)
    at internal/main/eval_string.js:23:3
$ node -p "require('vm').runInContext('URL', require('vm').createContext({ URL })) === URL"
true

We'd need a whole new implementation of URL (in case of Electron, Blink provides one, Node.js another one) to test this behaviour.

[ljharb]

ahhh, hm. what's the implementation of isURLInstance doing that would be robust to that scenario?

[aduh95]

ahhh, hm. what's the implementation of isURLInstance doing that would be robust to that scenario?

More info on that on #34622, I have actually very little knowledge myself on that topic 🙈

[ljharb]

Looking at that implementation, that's not "is URL instance", that's "has truthy href and origin properties", which is more like "is URL-like".

Is the intention to allow { href: 1, origin: 2 } as a valid URL-like thing?

[aduh95]

@ljharb I'd say it's probably OK, using such an object would fail anyway when converted to string. Hopefully the isURLInstance will be updated to get a finer test if the current implementation creates issues.

[aduh95]

I think this should be labeled Author ready if no one objects.

[DerekNonGeneric]

In case anyone is curious as to what isURLInstance does…

node/lib/internal/url.js

Lines 1420 to 1422 in 22c52aa

	function isURLInstance(fileURLOrPath) {
	return fileURLOrPath != null && fileURLOrPath.href && fileURLOrPath.origin;
	}

[nodejs-github-bot]

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

[github-actions]

Landed in d24eecd