-
Notifications
You must be signed in to change notification settings - Fork 29.8k
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
Conversation
This reverts commit 0dc485e.
Review requested:
|
deps/ada/ada.gyp
Outdated
['v8_enable_i18n_support==1', { | ||
'dependencies': [ | ||
'<(icu_gyp_path):icui18n', | ||
'<(icu_gyp_path):icuuc', |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
// 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 |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
test/wpt/status/url.json
Outdated
"percent-encoding.window.js": { | ||
"requires": ["small-icu"], |
There was a problem hiding this comment.
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.
I have not looked at the issue, but the current releases of ada do not use ICU (at all). In fact, ada has no dependency. |
I think reverting is not the fix for this. ICU based |
Superseded by #48873 |
This reverts commit 0dc485e.
Fixes: #48850
Fixes: #48855