doc,url: clarify domainTo* when built without ICU

[RaisinTen]

Continuation of: #35099

Signed-off-by: Darshan Sen raisinten@gmail.com

[aduh95]

Could you add a test for non-ICU builds that make sure the warning works please? (I think you can do that if you add if(common.hasIntl) common.skip('Non-ICU test only);)

[addaleax]

As in the original PR, I’m not convinced that this is actually useful – the vast majority of Node.js developers will never know that this is something that can happen – and that documentation changes should be the priority here (the URL docs don’t seem to mention this at all).

[addaleax]

This doesn’t seem quite right – why isn’t this passed along like other arguments? It doesn’t conceptually make a lot of sense to say that a URL is bound to a specific Environment instance, when it doesn’t associate with JS objects.

[addaleax]

If we do this, let’s be more specific about what actually happens here – “ToUnicode” and “ToASCII” are very generic names, and mirroring them 1:1 isn’t really useful to end users (it doesn’t even tell them that this is an operation on domain names that’s failing).

[RaisinTen]

As in the original PR, I’m not convinced that this is actually useful – the vast majority of Node.js developers will never know that this is something that can happen – and that documentation changes should be the priority here (the URL docs don’t seem to mention this at all).

@addaleax If we mention it in the docs that these functions return the same domain string unchanged if Node.js hasn't been compiled with ICU support, will it be enough or is the warning still necessary?

[addaleax]

@RaisinTen That is what I would do, yes – I can’t speak for you, of course, and you’re the one who is suggesting the warning here.

[RaisinTen]

Since the new URL() documentation mentions this already:

node/doc/api/url.md

Lines 174 to 175 in ab71af3

	This feature is only available if the `node` executable was compiled with
	[ICU][] enabled. If not, the domain names are passed through unchanged.

I too think it would be okay to make this just a doc change.
Updated, PTAL.

[aduh95]

Landed in bbab59d

[richardlau]

#35099 is semver-major so marking this one dont-land-on-v14.x.

[RaisinTen]

@richardlau This change actually solves the problem #35099 was trying to solve by just clarifying the docs by stating what has already been happening, so I think it is okay to land this on v14.x.