Skip to content

Commit

Permalink
Revert "url: drop ICU requirement for parsing hostnames"
Browse files Browse the repository at this point in the history
This reverts commit 0dc485e.
  • Loading branch information
aduh95 committed Jul 21, 2023
1 parent a39b8a2 commit 0560f54
Show file tree
Hide file tree
Showing 5 changed files with 77 additions and 5 deletions.
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',
],
}],
['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 @@ changes:
- 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/47339

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

View workflow job for this annotation

GitHub Actions / lint-pr-url

pr-url doesn't match the URL of the current PR.
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 @@ const myURL = new URL('https://測試');
// 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,6 +1043,9 @@ changes:
- 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/47339

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

View workflow job for this annotation

GitHub Actions / lint-pr-url

pr-url doesn't match the URL of the current PR.
description: ICU requirement is back.
-->

* `domain` {string}
Expand All @@ -1047,6 +1056,9 @@ invalid domain, the empty string is returned.

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,6 +1091,9 @@ changes:
- 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/47339

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

View workflow job for this annotation

GitHub Actions / lint-pr-url

pr-url doesn't match the URL of the current PR.
description: ICU requirement is back.
-->

* `domain` {string}
Expand All @@ -1089,6 +1104,9 @@ domain, the empty string is returned.

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 @@ console.log(myURL.origin);
// 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
// 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"],
"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"
}
}

0 comments on commit 0560f54

Please sign in to comment.