Skip to content

Commit

Permalink
url: drop ICU requirement for parsing hostnames
Browse files Browse the repository at this point in the history
PR-URL: #47339
Backport-PR-URL: #48345
Reviewed-By: Tiancheng "Timothy" Gu <timothygu99@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
  • Loading branch information
anonrig authored and danielleadams committed Jul 12, 2023
1 parent b395b16 commit 0dc485e
Show file tree
Hide file tree
Showing 5 changed files with 20 additions and 68 deletions.
19 changes: 2 additions & 17 deletions deps/ada/ada.gyp
@@ -1,6 +1,7 @@
{
'variables': {
'v8_enable_i18n_support%': 1,
'ada_sources': [ 'ada.cpp' ],
},
'targets': [
{
Expand All @@ -10,23 +11,7 @@
'direct_dependent_settings': {
'include_dirs': ['.'],
},
'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',
],
}],
]
'sources': [ '<@(ada_sources)' ]
},
]
}
25 changes: 15 additions & 10 deletions doc/api/url.md
Expand Up @@ -129,6 +129,13 @@ return `true`.

#### `new URL(input[, base])`

<!--
changes:
- version: REPLACEME
pr-url: https://github.com/nodejs/node/pull/47339
description: ICU requirement is removed.
-->

* `input` {string} The absolute or relative input URL to parse. If `input`
is relative, then `base` is required. If `input` is absolute, the `base`
is ignored. If `input` is not a string, it is [converted to a string][] first.
Expand Down Expand Up @@ -172,9 +179,6 @@ 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 @@ -1029,6 +1033,10 @@ for (const [name, value] of params) {
added:
- v7.4.0
- v6.13.0
changes:
- version: REPLACEME
pr-url: https://github.com/nodejs/node/pull/47339
description: ICU requirement is removed.
-->

* `domain` {string}
Expand All @@ -1039,9 +1047,6 @@ 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 @@ -1070,6 +1075,10 @@ console.log(url.domainToASCII('xn--iñvalid.com'));
added:
- v7.4.0
- v6.13.0
changes:
- version: REPLACEME
pr-url: https://github.com/nodejs/node/pull/47339
description: ICU requirement is removed.
-->

* `domain` {string}
Expand All @@ -1080,9 +1089,6 @@ 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 @@ -1725,7 +1731,6 @@ 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: 2 additions & 7 deletions lib/internal/idna.js
@@ -1,9 +1,4 @@
'use strict';

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 };
}
const { domainToASCII, domainToUnicode } = require('internal/url');
module.exports = { toASCII: domainToASCII, toUnicode: domainToUnicode };
14 changes: 1 addition & 13 deletions test/benchmark/test-benchmark-url.js
@@ -1,18 +1,6 @@
'use strict';

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');
}
require('../common');

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

Expand Down
21 changes: 0 additions & 21 deletions test/wpt/status/url.json
@@ -1,40 +1,19 @@
{
"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 0dc485e

Please sign in to comment.