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.

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

[v18.x] Revert "url: drop ICU requirement for parsing hostnames" #48869

Closed
Closed
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
19 changes: 17 additions & 2 deletions deps/ada/ada.gyp
Original file line number Diff line number Diff line change
@@ -1,7 +1,6 @@
{
'variables': {
'v8_enable_i18n_support%': 1,
'ada_sources': [ 'ada.cpp' ],
},
'targets': [
{
Expand All @@ -11,7 +10,23 @@
'direct_dependent_settings': {
'include_dirs': ['.'],
},
'sources': [ '<@(ada_sources)' ]
'sources': ['ada.cpp'],
'conditions': [
['v8_enable_i18n_support==0', {
'defines': ['ADA_HAS_ICU=0'],
}],
['v8_enable_i18n_support==1', {
'dependencies': [
'<(icu_gyp_path):icui18n',
'<(icu_gyp_path):icuuc',
Copy link
Member

Choose a reason for hiding this comment

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

This file shouldn’t be reverted. This configurations are for Ada v1.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think it makes sense to land a clean revert of the commit causing the issue, and then we have all the time we need to land a follow up commit that tidy things up, wdyt?

],
}],
['OS=="win" and v8_enable_i18n_support==1', {
'dependencies': [
'<(icu_gyp_path):icudata',
],
}],
]
},
]
}
19 changes: 19 additions & 0 deletions doc/api/url.md
Original file line number Diff line number Diff line change
Expand Up @@ -134,6 +134,9 @@
- version: v18.17.0
pr-url: https://github.com/nodejs/node/pull/47339
description: ICU requirement is removed.
- version: REPLACEME
pr-url: https://github.com/nodejs/node/pull/48869
description: ICU requirement is back.
-->

* `input` {string} The absolute or relative input URL to parse. If `input`
Expand Down Expand Up @@ -179,6 +182,9 @@
// https://xn--g6w251d/
```

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

In cases where it is not known in advance if `input` is an absolute URL
and a `base` is provided, it is advised to validate that the `origin` of
the `URL` object is what is expected.
Expand Down Expand Up @@ -1037,7 +1043,10 @@
- version: v18.17.0
pr-url: https://github.com/nodejs/node/pull/47339
description: ICU requirement is removed.
- version: REPLACEME
pr-url: https://github.com/nodejs/node/pull/48869
description: ICU requirement is back.
-->

Check warning on line 1049 in doc/api/url.md

View workflow job for this annotation

GitHub Actions / lint-js-and-md

Items in `changes` list are not in order

* `domain` {string}
* Returns: {string}
Expand All @@ -1047,6 +1056,9 @@

It performs the inverse operation to [`url.domainToUnicode()`][].

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

```mjs
import url from 'node:url';

Expand Down Expand Up @@ -1079,7 +1091,10 @@
- version: v18.17.0
pr-url: https://github.com/nodejs/node/pull/47339
description: ICU requirement is removed.
- version: REPLACEME
pr-url: https://github.com/nodejs/node/pull/48869
description: ICU requirement is back.
-->

Check warning on line 1097 in doc/api/url.md

View workflow job for this annotation

GitHub Actions / lint-js-and-md

Items in `changes` list are not in order

* `domain` {string}
* Returns: {string}
Expand All @@ -1089,6 +1104,9 @@

It performs the inverse operation to [`url.domainToASCII()`][].

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

```mjs
import url from 'node:url';

Expand Down Expand Up @@ -1731,6 +1749,7 @@
// Prints https://xn--1xa.example.com
```

[ICU]: intl.md#options-for-building-nodejs
[Punycode]: https://tools.ietf.org/html/rfc5891#section-4.4
[WHATWG URL]: #the-whatwg-url-api
[WHATWG URL Standard]: https://url.spec.whatwg.org/
Expand Down
9 changes: 7 additions & 2 deletions lib/internal/idna.js
Original file line number Diff line number Diff line change
@@ -1,4 +1,9 @@
'use strict';

const { domainToASCII, domainToUnicode } = require('internal/url');
module.exports = { toASCII: domainToASCII, toUnicode: domainToUnicode };
if (internalBinding('config').hasIntl) {
const { toASCII, toUnicode } = internalBinding('icu');
module.exports = { toASCII, toUnicode };
} else {
const { domainToASCII, domainToUnicode } = require('internal/url');
module.exports = { toASCII: domainToASCII, toUnicode: domainToUnicode };
}
14 changes: 13 additions & 1 deletion test/benchmark/test-benchmark-url.js
Original file line number Diff line number Diff line change
@@ -1,6 +1,18 @@
'use strict';

require('../common');
const common = require('../common');

// TODO(@anonrig): Remove this check when Ada removes ICU requirement.
if (!common.hasIntl) {
// A handful of the benchmarks fail when ICU is not included.
// ICU is responsible for ignoring certain inputs from the hostname
Copy link
Member

Choose a reason for hiding this comment

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

This file shouldn’t be reverted as well.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think it should be, url.parse depends on ICU if this PR lands.

// and without it, it is not possible to validate the correctness of the input.
// DomainToASCII method in Unicode specification states which characters are
// ignored and/or remapped. Doing this outside of the scope of DomainToASCII,
// would be a violation of the WHATWG URL specification.
// Please look into: https://unicode.org/reports/tr46/#ProcessingStepMap
common.skip('missing Intl');
}

const runBenchmark = require('../common/benchmark');

Expand Down
21 changes: 21 additions & 0 deletions test/wpt/status/url.json
Original file line number Diff line number Diff line change
@@ -1,19 +1,40 @@
{
"toascii.window.js": {
"requires": ["small-icu"]
},
"percent-encoding.window.js": {
"requires": ["small-icu"],
Copy link
Member

Choose a reason for hiding this comment

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

This file should not be reverted.

"skip": "TODO: port from .window.js"
},
"historical.any.js": {
"requires": ["small-icu"],
"fail": {
"expected": [
"URL: no structured serialize/deserialize support",
"URLSearchParams: no structured serialize/deserialize support"
]
}
},
"urlencoded-parser.any.js": {
"requires": ["small-icu"]
},
"url-constructor.any.js": {
"requires": ["small-icu"]
},
"url-origin.any.js": {
"requires": ["small-icu"]
},
"url-setters.any.js": {
"requires": ["small-icu"]
},
"url-setters-a-area.window.js": {
"skip": "already tested in url-setters.any.js"
},
"IdnaTestV2.window.js": {
"requires": ["small-icu"]
},
"javascript-urls.window.js": {
"required": ["small-icu"],
"skip": "requires document.body reference"
}
}
Loading