url: improve isURL detection

[anonrig]

Previously we were checking for protocol and origin attributes, but due to the lazy-loading nature of Ada 2.0 with url_aggregator we shifted from checking for origin. This was also breaking the detection of the legacy URL parse function. This PR fixes that in the least impactful way possible, by checking if auth property is defined.

Fixes #47624

cc @nodejs/url @Trott

[nodejs-github-bot]

Review requested:

• @nodejs/url

[nodejs-github-bot]

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

[nodejs-github-bot]

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

[nodejs-github-bot]

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

[Trott]

Can we add a test that checks without --expose-internals? Something like this would work, I think:

'use strict';

const common = require('../common');
if (!common.hasCrypto) { common.skip('missing crypto'); }
const assert = require('assert');

const opt = require('url').parse('https://httpbin.org/get');
require('https')
  .request(
    { ...opt, path: opt.path + '?A=B' },
    (s) => s.once('data',
                (d) => assert.ok(d.toString('UTF8').includes('https://httpbin.org/get?A=B'))))
  .end();

This will test the behavior the user experiences rather than internal implementation details.

It can probably be improved (e.g., to aggregate the data chunks rather than assume it will all be in the first chunk), but you get the idea.

[anonrig]

Of course, @Trott can you block the pull request to avoid accidentaly merging this pr?

[Trott]

(Removing commit queue label pending "do we need test that checks the public API/behavior" resolution.)

[Trott]

(Removing commit queue label pending "do we need test that checks the public API/behavior" resolution.)

And now putting it back because we can always add the test in a subsequent PR.

[VoltrexKeyva]

Suggested change

	* We check for auth attribute to distinguish legacy url instance with
	* We check for auth attribute to distinguish legacy URL instance with

For consistency.

[Trott]

URL is the name of the WHATWG URL object. Perhaps to avoid confusion, it should be urlObject here to be consistent with https://nodejs.org/dist/latest-v20.x/docs/api/url.html#legacy-urlobject?

Suggested change

	* We check for auth attribute to distinguish legacy url instance with
	* We check for auth attribute to distinguish legacy urlObject instance with

[Trott]

Needs a public API test.

[Trott]

Of course, @Trott can you block the pull request to avoid accidentaly merging this pr?

Sure. Done.

[Trott]

My test suggestion is not great as it depends on the behavior of an external service. I'm going to unblock this. We can always add another test later.

[nodejs-github-bot]

Landed in 528aaca