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.

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

Throw RangeError instead of TypeError for format with non-object types #128

Closed
Constellation opened this issue Oct 3, 2022 · 2 comments · Fixed by #130
Closed

Throw RangeError instead of TypeError for format with non-object types #128

Constellation opened this issue Oct 3, 2022 · 2 comments · Fixed by #130
Labels
consensus We reached a consensus in a discussion meeting, through email or the issue discussion

Comments

@Constellation
Copy link
Member

Constellation commented Oct 3, 2022 •
edited
Loading

We will integrate Temporal duration related changes in the future, and it will change the current format function's TypeError to RangeError.
So, how about making it RangeError from the beginning? I think the rationale is that, "Intl.Duration#format can format Duration described as a String in a particular syntax, but in this spec, no syntax is accepted, so it is always throwing a RangeError".

var fmt = new Intl.DurationFormat('en');
fmt.format("");  // TypeError. But if Temporal is integrated, it will become RangeError
@sffc sffc added the Meeting Discussion Need to be discussed in one of the upcoming meetings label Oct 5, 2022
@sffc
Copy link
Collaborator

sffc commented Nov 3, 2022

TG2 discussion: https://github.com/tc39/ecma402/blob/master/meetings/notes-2022-11-03.md#throw-rangeerror-instead-of-typeerror-for-format-with-non-object-types-128

Conclusion: RangeError, strings only.

@sffc sffc added consensus We reached a consensus in a discussion meeting, through email or the issue discussion and removed Meeting Discussion Need to be discussed in one of the upcoming meetings labels Nov 3, 2022
ryzokuken added a commit to ryzokuken/proposal-intl-duration-format that referenced this issue Nov 25, 2022
@ryzokuken
normative: throw rangeerror with strings
88f6dfc 
Throw a RangeError on string inputs in ToDurationRecord for future
Temporal compatibility.

Fixes: tc39#128
@ryzokuken ryzokuken mentioned this issue Nov 25, 2022
Merged
ryzokuken added a commit to ryzokuken/proposal-intl-duration-format that referenced this issue Dec 9, 2022
@ryzokuken
normative: throw rangeerror with strings
e7b1e8f 
Throw a RangeError on string inputs in ToDurationRecord for future
Temporal compatibility.

Fixes: tc39#128
ryzokuken added a commit that referenced this issue Dec 9, 2022
@ryzokuken
normative: throw rangeerror with strings
2546080 
Throw a RangeError on string inputs in ToDurationRecord for future
Temporal compatibility.

Fixes: #128
@FrankYFTang
Copy link
Collaborator

We also need to change the following test to sync with it. @romulocintra

intl402/DurationFormat/prototype/formatToParts/invalid-arguments-throws.js
intl402/DurationFormat/prototype/format/invalid-arguments-throws.js

@Constellation Constellation mentioned this issue Jan 24, 2023
Merged
webkit-early-warning-system pushed a commit to Constellation/WebKit that referenced this issue Jan 25, 2023
@Constellation
[JSC] Update Intl.DurationFormat based on Jan ECMA402 consensus
4bfba24 
https://bugs.webkit.org/show_bug.cgi?id=251115
rdar://104582024

Reviewed by Mark Lam.

This patch upgrades Intl.DurationFormat implementation to align it to Jan ECMA402 meeting consensus.
The changes are three-fold.

1. format and formatToParts should accept String too. And throwing a range error when string is not valid[1].
2. formatToParts should use singular form of unit names instead of plural form.
3. formatToParts should split each unit's representation to make numeric part and unit part accessible[3]. Previously,
   we just grouped "1 hour" as "hour" part. But after this, we split it into "1" integer, " " literal, and "hour" unit,
   with unit = "hour".

[1]: tc39/proposal-intl-duration-format#128
[2]: tc39/proposal-intl-duration-format#44
[3]: tc39/proposal-intl-duration-format#55

* JSTests/stress/intl-durationformat-format-to-parts.js:
(Intl.DurationFormat.shouldBe.JSON.stringify.fmt.formatToParts):
(Intl.DurationFormat.shouldBeOneOf):
* JSTests/stress/intl-durationformat.js:
(test):
* Source/JavaScriptCore/runtime/IntlDurationFormat.cpp:
(JSC::collectElements):
(JSC::IntlDurationFormat::formatToParts const):
* Source/JavaScriptCore/runtime/IntlDurationFormatPrototype.cpp:
(JSC::JSC_DEFINE_HOST_FUNCTION):

Canonical link: https://commits.webkit.org/259317@main
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
consensus We reached a consensus in a discussion meeting, through email or the issue discussion
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

normative: throw rangeerror with strings ryzokuken/proposal-intl-duration-format
3 participants
@Constellation @sffc @FrankYFTang