Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

doc,url: clarify domainTo* when built without ICU #38789

Merged
merged 1 commit into from May 26, 2021
Merged

doc,url: clarify domainTo* when built without ICU #38789

merged 1 commit into from May 26, 2021

Conversation

RaisinTen
Copy link
Contributor

Continuation of: #35099

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

@github-actions github-actions bot added c++ Issues and PRs that require attention from people who are familiar with C++. needs-ci PRs that need a full CI run. labels May 23, 2021
@aduh95
Copy link
Contributor

aduh95 commented May 23, 2021

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 addaleax added the semver-major PRs that contain breaking changes and should be released in the next major version. label May 24, 2021
addaleax
addaleax requested changes May 24, 2021
View reviewed changes
Copy link
Member

@addaleax addaleax left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

src/node_url.h Outdated
@@ -183,6 +183,7 @@ class URL {
URL() : URL("") {}

private:
Environment* env_ = nullptr;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

src/node_url.cc Outdated
const std::string& input,
std::string* output) {
ProcessEmitWarning(env,
"Cannot convert to ASCII when intl is disabled");
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Copy link
Contributor Author

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
Copy link
Member

@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
Copy link
Contributor Author

RaisinTen commented May 25, 2021 •
edited

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.

@RaisinTen RaisinTen force-pushed the url/warn-when-operation-requires-ICU branch from e31fd7a to 17b38d7 Compare May 25, 2021 08:25
@RaisinTen RaisinTen changed the title url: warn when operation requires ICU doc,url: clarify domainTo* when built without ICU May 25, 2021
@addaleax addaleax added doc Issues and PRs related to the documentations. url Issues and PRs related to the legacy built-in url module. and removed c++ Issues and PRs that require attention from people who are familiar with C++. semver-major PRs that contain breaking changes and should be released in the next major version. labels May 26, 2021
@RaisinTen @aduh95
doc,url: clarify domainTo* when built without ICU
bbab59d 
Continuation of: nodejs#35099

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

PR-URL: nodejs#38789
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
@aduh95 aduh95 force-pushed the url/warn-when-operation-requires-ICU branch from 17b38d7 to bbab59d Compare May 26, 2021 13:57
@aduh95
Copy link
Contributor

aduh95 commented May 26, 2021

Landed in bbab59d

@aduh95 aduh95 merged commit bbab59d into nodejs:master May 26, 2021
@RaisinTen RaisinTen deleted the url/warn-when-operation-requires-ICU branch May 26, 2021 13:58
danielleadams pushed a commit that referenced this pull request May 31, 2021
@RaisinTen @danielleadams
doc,url: clarify domainTo* when built without ICU
0c063a1 
Continuation of: #35099

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

PR-URL: #38789
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
@danielleadams danielleadams mentioned this pull request May 31, 2021
Merged
@richardlau
Copy link
Member

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

@RaisinTen
Copy link
Contributor Author

RaisinTen commented Jul 17, 2021 •
edited

@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.

richardlau pushed a commit that referenced this pull request Jul 22, 2021
@RaisinTen @richardlau
doc,url: clarify domainTo* when built without ICU
64140af 
Continuation of: #35099

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

PR-URL: #38789
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
richardlau pushed a commit that referenced this pull request Jul 22, 2021
@RaisinTen @richardlau
doc,url: clarify domainTo* when built without ICU
2ff671e 
Continuation of: #35099

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

PR-URL: #38789
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
@richardlau richardlau mentioned this pull request Jul 22, 2021
Merged
foxxyz pushed a commit to foxxyz/node that referenced this pull request Oct 18, 2021
@RaisinTen @foxxyz
doc,url: clarify domainTo* when built without ICU
d81846d 
Continuation of: nodejs#35099

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

PR-URL: nodejs#38789
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@addaleax addaleax addaleax approved these changes

@aduh95 aduh95 aduh95 approved these changes

@jasnell jasnell Awaiting requested review from jasnell

Assignees
No one assigned
Labels
doc Issues and PRs related to the documentations. needs-ci PRs that need a full CI run. url Issues and PRs related to the legacy built-in url module.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@RaisinTen @aduh95 @addaleax @richardlau @jasnell