Skip to content

Add tests for Intl.Locale.prototype.variants #4474

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
Merged
Ms2ger merged 2 commits into from
Jun 4, 2025
Merged

Add tests for Intl.Locale.prototype.variants #4474

Ms2ger merged 2 commits into from
Jun 4, 2025

Conversation

@gibson042
Copy link
Member

@gibson042 gibson042 commented May 8, 2025

@gibson042 gibson042 requested a review from a team as a code owner May 8, 2025 22:01
@gibson042 gibson042 mentioned this pull request May 9, 2025
Merged
@ptomato ptomato added needs review awaiting consensus This needs committee consensus before it can be eligible to be merged. labels May 14, 2025
@gibson042 gibson042 force-pushed the ecma402-960-locale-getvariants branch from 8475efc to bb37a1e Compare May 29, 2025 22:16
@gibson042
Copy link
Member Author

gibson042 commented May 29, 2025

tc39/ecma402#960 reached consensus in TC39 plenary, and has been merged into ECMA-402. This can be merged as soon as it has a review (which should be relatively low effort, especially if going by commit-by-commit).

ptomato
ptomato approved these changes May 30, 2025
View reviewed changes
test/intl402/Locale/constructor-options-variants-valid.js Outdated
];
for (const [lang, variants, baseName] of validVariantsOptions) {
let options = { variants };
let optionsRepr = `{variants: ${variants == null ? variants : `"${variants}"`}}`;
Copy link
Contributor

@ptomato ptomato May 30, 2025

Choose a reason for hiding this comment

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

Nitpick: the == operator here is very subtle but actually significant and needed. I overlooked it in several reads, and thought the code looked wrong. Would there be a more obvious way to write it?

anba
anba suggested changes May 30, 2025
View reviewed changes
Copy link
Contributor

@anba anba left a comment

Choose a reason for hiding this comment

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

And maybe also test canonicalisation works correctly when the variants option is used:

  1. Variant subtags are correctly sorted:
js> new Intl.Locale("en", {variants: "spanglis-oxendict"}).toString()
"en-oxendict-spanglis"
  1. Deprecated language tags are canonicalised to their modern form:
js> new Intl.Locale("cel", {variants: "gaulish"}).toString()           
"xtg"

test/intl402/Locale/constructor-options-variants-invalid.js Outdated
"3xy",
"abcd",
"12345",
"5wxyz",
Copy link
Contributor

@anba anba May 30, 2025

Choose a reason for hiding this comment

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

"12345" and "5wxyz" are both valid variant subtags.

test/intl402/Locale/constructor-options-variants-invalid.js

// Value contains more than just the 'variants' production.
"GB-scouse",
];
Copy link
Contributor

@anba anba May 30, 2025

Choose a reason for hiding this comment

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

Can we also test for leading, trailing, and consecutive "-"?

const alsoInvalid = [
  "-",
  "-spanglis",
  "spanglis-",
  "-spanglis-oxendict",
  "spanglis-oxendict-",
  "spanglis--oxendict",
];

Copy link
Contributor

@anba anba May 30, 2025

Choose a reason for hiding this comment

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

And test that duplicate variants throw:

js> new Intl.Locale("en", {variants: "spanglis-spanglis"})
typein:1:1 RangeError: duplicate variant subtag

@gibson042
Copy link
Member Author

gibson042 commented May 30, 2025

Thanks for the great comments. This is ready for re-review.

@gibson042 gibson042 requested a review from anba June 2, 2025 18:15
@anba
Copy link
Contributor

anba commented Jun 4, 2025

LGTM! I've confirmed all tests pass with the patches from https://bugzilla.mozilla.org/show_bug.cgi?id=1970161 applied.

@Ms2ger Ms2ger force-pushed the ecma402-960-locale-getvariants branch from 1d70af5 to bbdd4a8 Compare June 4, 2025 08:55
gibson042 added 2 commits June 4, 2025 10:56
@gibson042 @Ms2ger
Add tests for Intl.Locale.prototype.variants
9da80fa 
Ref tc39/ecma402#960
@gibson042 @Ms2ger
Improve test/intl402/Locale/constructor-getter-order.js
bbdd4a8 
* Add a helper for concision
* Specify properties in a different order than the one used for reading
@Ms2ger Ms2ger removed needs review awaiting consensus This needs committee consensus before it can be eligible to be merged. labels Jun 4, 2025
@Ms2ger Ms2ger enabled auto-merge (rebase) June 4, 2025 08:56
@Ms2ger Ms2ger merged commit a450164 into tc39:main Jun 4, 2025
11 checks passed
sosukesuzuki added a commit to sosukesuzuki/WebKit that referenced this pull request Jun 20, 2025
@sosukesuzuki
[JSC] Implement Intl.Locale.prototype.variants getter
c6bde4d 
https://bugs.webkit.org/show_bug.cgi?id=294755

Reviewed by NOBODY (OOPS!).

tc39/ecma402#960 added `Intl.Locale.prototype.variants`
to the ECMAScript specification. The test262 test suite has also been updated [1].

This patch implements `Intl.Locale.prototype.variants` for JSC.

[1]: tc39/test262#4474

* JSTests/stress/intl-locale-prototype-variants.js: Added.
(shouldBe):
(shouldThrow):
* JSTests/test262/expectations.yaml:
* Source/JavaScriptCore/runtime/CommonIdentifiers.h:
* Source/JavaScriptCore/runtime/IntlLocale.cpp:
(JSC::LocaleIDBuilder::overrideLanguageScriptRegionVariants):
(JSC::IntlLocale::initializeLocale):
(JSC::IntlLocale::variants):
(JSC::LocaleIDBuilder::overrideLanguageScriptRegion): Deleted.
* Source/JavaScriptCore/runtime/IntlLocale.h:
* Source/JavaScriptCore/runtime/IntlLocalePrototype.cpp:
(JSC::JSC_DEFINE_CUSTOM_GETTER):
@sosukesuzuki sosukesuzuki mentioned this pull request Jun 20, 2025
Merged
webkit-commit-queue pushed a commit to sosukesuzuki/WebKit that referenced this pull request Jun 21, 2025
@sosukesuzuki
[JSC] Implement Intl.Locale.prototype.variants getter
b8b32fe 
https://bugs.webkit.org/show_bug.cgi?id=294755

Reviewed by Yusuke Suzuki.

tc39/ecma402#960 added `Intl.Locale.prototype.variants`
to the ECMAScript specification. The test262 test suite has also been updated [1].

This patch implements `Intl.Locale.prototype.variants` for JSC.

[1]: tc39/test262#4474

* JSTests/stress/intl-locale-prototype-variants.js: Added.
(shouldBe):
(shouldThrow):
* JSTests/test262/expectations.yaml:
* Source/JavaScriptCore/runtime/CommonIdentifiers.h:
* Source/JavaScriptCore/runtime/IntlLocale.cpp:
(JSC::LocaleIDBuilder::overrideLanguageScriptRegionVariants):
(JSC::IntlLocale::initializeLocale):
(JSC::IntlLocale::variants):
(JSC::LocaleIDBuilder::overrideLanguageScriptRegion): Deleted.
* Source/JavaScriptCore/runtime/IntlLocale.h:
* Source/JavaScriptCore/runtime/IntlLocalePrototype.cpp:
(JSC::JSC_DEFINE_CUSTOM_GETTER):

Canonical link: https://commits.webkit.org/296467@main
hubot pushed a commit to v8/v8 that referenced this pull request Oct 18, 2025
@FrankYFTang
[intl] add Intl.Locale.prototype get variants
7384de2 
https://tc39.es/ecma402/#sec-Intl.Locale.prototype.variants

Added to the spec by tc39/ecma402#960
Test code added in tc39/test262#4474
feature https://chromestatus.com/feature/4709921706868736
I2P https://groups.google.com/a/chromium.org/g/blink-dev/c/NCT4pPJ_Uz8/m/G62K-m6CAgAJ

Bug: 450083673
Change-Id: I49a4af9f9e75a18efb3b67f546f971d4c24e0bb0
Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/7018878
Reviewed-by: Leszek Swirski <[email protected]>
Reviewed-by: Olivier Flückiger <[email protected]>
Commit-Queue: Frank Tang <[email protected]>
Cr-Commit-Position: refs/heads/main@{#103188}
hubot pushed a commit to v8/v8 that referenced this pull request Oct 21, 2025
@FrankYFTang
[Intl] Stage Intl.Locale.prototype.variants
b3cd2f8 
https://tc39.es/ecma402/#sec-Intl.Locale.prototype.variants

Added to the spec by tc39/ecma402#960
Test code added in tc39/test262#4474
feature https://chromestatus.com/feature/4709921706868736
I4DT https://groups.google.com/a/chromium.org/g/blink-dev/c/i6QEHcH_fiI/m/4xUyZbSvAAAJ

Bug: 450083673
Change-Id: I987b3066726323550d098222e50da999c145495c
Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/7064530
Commit-Queue: Frank Tang <[email protected]>
Reviewed-by: Rezvan Mahdavi Hezaveh <[email protected]>
Cr-Commit-Position: refs/heads/main@{#103233}
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@ptomato ptomato ptomato approved these changes

@anba anba Awaiting requested review from anba

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@gibson042 @anba @ptomato @Ms2ger