url: ensure search property is consistently null vs empty

[JustinBeckwith]

Fixes: #13404

Checklist

• make -j4 test (UNIX), or vcbuild test (Windows) passes
• tests and/or benchmarks are included
• commit message follows commit guidelines

Affected core subsystem(s)

url

[TimothyGu]

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

[Trott]

We'll probably want to do a CITGM run to get an idea if this breaks stuff in the ecosystem. If so, we should mark it semver-major (and, of course, carefully consider if the breakage is worth it).

Seems good to me, but I'll defer to people more familiar with the url module (like @TimothyGu who's already reviewed it).

[tniessen]

I guess my opinion is obvious after creating #13404 :)

[tniessen]

CI failure on arm is unrelated, see #13603.

[refack]

short summary of previous discussion:

1. disparity:

> url.parse('http://example.com/', false).search
null
> url.parse('http://example.com/', true).search
''

2. other N/A properties are set to null, not ''

[refack]

CITGM looks clean: https://ci.nodejs.org/view/Node.js-citgm/job/citgm-smoker/868/

[Trott]

LGTM

[Trott]

Pinging @nodejs/ctc since it's labeled semver-major. (A case could certainly be made that it's semver-patch, but I'm definitely OK with being cautious on this one.)

[indutny]

@Trott how many releases had "bad" behavior?

[indutny]

Looks like up to v4, at least. I'd go with semver major then.

[mcollina]

LGTM as semver-major

[jasnell]

LGTM as semver-major

[tniessen]

New CI after #13603 was fixed: https://ci.nodejs.org/job/node-test-pull-request/8625/

[refack]

arm-fanned had an infra hiccup, rerunning: https://ci.nodejs.org/job/node-test-commit-arm-fanned/9386/

[jasnell]

CITGM: https://ci.nodejs.org/view/Node.js-citgm/job/citgm-smoker/876/

[refack]

CITGM: https://ci.nodejs.org/view/Node.js-citgm/job/citgm-smoker/876/

Had to kill macOS because of known bug nodejs/citgm#426
Other then that CITGM clean ✔️

[jasnell]

Landed in c88ba03

[styfle]

@JustinBeckwith @jasnell This probably broke userland code like espadrine/sc#65 however I don't see a mention of History in the docs for url.parse. Should this be added?

[TimothyGu]

@styfle PR/issue welcome.

[jasnell]

Yep, a mention in the doc changelogs would be excellent

[styfle]

@TimothyGu @jasnell I made a PR.

But after thinking through this, I believe it's better to change null to the empty string.
My reason is that the browser seems to never return null and returns the empty string.
For example, this page window.location.search === '' because there is no query string.
And I like the idea of never using null unless you have to.
In typescript, the type is string | null but if it was never null, you could change the type to string and you would never have to check before doing search.split(',') or search.slice(0).

@JustinBeckwith Why did you decide to use null instead of making both scenarios return empty string?

[JustinBeckwith]

I generally don't like returning empty string when we're trying to convey that the value is 'unset'. Those to me mean very different things. That having been said - I was handed this as part of the code & learn at node.js interactive :) I don't think either solution is necessarily wrong, but there would be a cost to flip-flopping.