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.

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

Normative: Add fractionalSecondDigits option #347

Merged
merged 4 commits into from
Jan 5, 2021
Merged

Conversation

FrankYFTang
Copy link
Contributor

@FrankYFTang FrankYFTang commented May 21, 2019

Address #300 @HolgerJeromin
UPDATED 6/5/2019 1300 PST

Format Millisecond

let t = new Date("2019-05-20T07:00:23.123");
var dtf = new Intl.DateTimeFormat(
   "en", {hour: "numeric", minute: "numeric", second: "numeric", fractionalSecondDigits: 3});
dtf.format(t);  // "7:00:23.123 AM" 
dtf.formatToParts(t);
// [{type: "hour", value: "7"},
//  {type: "literal", value: ":"}, 
//  {type: "minute", value: "00"}, 
//  {type: "literal", value: ":"}, 
//  {type: "second", value: "23"}, 
//  {type: "literal", value: "."}, 
//  {type: "fractionalSecond", value: "123"}, 
//  {type: "literal", value: " "}, 
//  {type: "dayPeriod", value: "AM"}]
dtf = new Intl.DateTimeFormat(
   "en", {hour: "numeric", minute: "numeric", second: "numeric", fractionalSecondDigits: 1});
dtf.format(t));  // "7:00:23.1 AM"

@FrankYFTang
Copy link
Contributor Author

@FrankYFTang
Copy link
Contributor Author

@FrankYFTang FrankYFTang added c: datetime Component: dates, times, timezones enhancement s: discuss Status: TG2 must discuss to move forward Small Smaller change solvable in a Pull Request labels May 21, 2019
@FrankYFTang FrankYFTang changed the title Normative: add millisecondDigits support Normative: Add millisecondDigits option May 21, 2019
spec/datetimeformat.html Outdated Show resolved Hide resolved
@HolgerJeromin
Copy link

Thanks. This will solve my use case.
It there a possibility to detect this feature (other than trying formatToParts and search in the result)?

@FrankYFTang
Copy link
Contributor Author

FrankYFTang commented May 22, 2019

Thanks. This will solve my use case.
It there a possibility to detect this feature (other than trying formatToParts and search in the result)?

Good point. I didn't think about that while I make the spec change but I believe we already have facility to do that. Here is one sniffing function I wrote to test in my prototype to tell it is available or not (notice, with my propose spec, you do need to add the second option too.)

function IsMillisecondDigitsOptionAvaiable() { 
  return (new Intl.DateTimeFormat(
    undefined, {second: "numeric"})).resolvedOptions().millisecondDigits !== undefined; 
}

Below is what I run in my prototype on v8, --harmony_intl_dateformat_millisecond_digits turn the feature on.
Run with the feature:

ftang@ftang4:~/v8/v8$ out/x64.release/d8 --harmony_intl_dateformat_millisecond_digits
V8 version 7.6.0 (candidate)
d8> (new Intl.DateTimeFormat(undefined, {second: "numeric"}).resolvedOptions()).millisecondDigits !== undefined
true

Run without the feature (no --harmony_intl_dateformat_millisecond_digits flag)

ftang@ftang4:~/v8/v8$ out/x64.release/d8
V8 version 7.6.0 (candidate)
d8> (new Intl.DateTimeFormat(undefined, {second: "numeric"}).resolvedOptions()).millisecondDigits !== undefined
false

But this is just based on the current PR. Don't count on it until we all agree this PR is good enough.

Notice in the current shape of the PR you need to pass in {second: "numeric"} as option because we won't check the "millisecondDigits" unless second is specified. I am not 100% sure that is right. Maybe that option should be read unconditionally and just got ignored if we are not formatting second?

@FrankYFTang
Copy link
Contributor Author

@littledan could you take a look again, I change the PartitionDateTimePattern to use FormatNumber. Please see does it match what you suggest or is there a better way to spec that.

@@ -105,6 +105,8 @@ <h1>InitializeDateTimeFormat ( _dateTimeFormat_, _locales_, _options_ )</h1>
1. Let _prop_ be the name given in the Property column of the row.
1. Let _value_ be ? GetOption(_options_, _prop_, `"string"`, &laquo; the strings given in the Values column of the row &raquo;, *undefined*).
1. Set _opt_.[[&lt;_prop_&gt;]] to _value_.
1. If _opt_.[[Second]] is not *undefined*, then
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@littledan , @sffc @fabalbon and everyone:
Do you think we should set opt.[[MillisecondDigits]] only if opt.[[Second]] is not undefined or it should be set unconditionally?

Choose a reason for hiding this comment

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

I think we should set opt.[[MillisecondDigits]] regardless. Like @HolgerJeromin commented, second can be set without hour. In InitializeDateTimeFormat,opt.[[Day]] will be set without opt.[[Month]].

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok, I just change the PR, then the detection can be simplified as

function IsMillisecondDigitsOptionAvaiable() { 
  return (new Intl.DateTimeFormat()).resolvedOptions().millisecondDigits !== undefined; 
}

Choose a reason for hiding this comment

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

Off topic (?): The detection code is surprising to me. Stable Chrome returns:

(new Intl.DateTimeFormat()).resolvedOptions() // results in
{
	calendar: "gregory",
	day: "numeric",
	locale: "en-US",
	month: "numeric",
	numberingSystem: "latn",
	timeZone: "Europe/Berlin",
	year: "numeric"
}

Why does a Chrome with your patches adds millisecondDigits?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Off topic (?): The detection code is surprising to me. Stable Chrome returns:

(new Intl.DateTimeFormat()).resolvedOptions() // results in
{
	calendar: "gregory",
	day: "numeric",
	locale: "en-US",
	month: "numeric",
	numberingSystem: "latn",
	timeZone: "Europe/Berlin",
	year: "numeric"
}

Why does a Chrome with your patches adds millisecondDigits?

not sure your question is asking how my patch in chrome work (so I can answer you by walking you through the C++ code) or how does the spec change work (so I can answer you by walking you through the changed spec).

@HolgerJeromin
Copy link

It there a possibility to detect this feature (other than trying formatToParts and search in the result)?

resolvedOptions

I did not used the resolvedOptions before. But yes, seems like a working (and not too ugly) detection.

Do you think we should set opt.[[MillisecondDigits]] only if opt.[[Second]] is not undefined or it should be set unconditionally?

Hard to tell. Right now the following (useless?) example is working without a problem. So the current philosophy is to allow all combination.
A minute is a kind of fraction of an hour, too. I am not sure where there would be a benefit for this restriction. So I would opt to drop the restriction for milliseconds, too.

new Intl.DateTimeFormat('en-us', {
  weekday: 'long',
  year: 'numeric',   
  day: 'numeric',   
  second: 'numeric',
  hour12: true,
  timeZone: 'UTC'
}).format(Date.UTC(2012, 11, 17, 3, 0, 42)); //  => "17 Monday 2012, 42"

@FrankYFTang

This comment has been minimized.

@leobalter

This comment has been minimized.

@pipobscure
Copy link

pipobscure commented Jun 5, 2019

As per live discussion at TC39-Berlin:

I think we would be well advised to name this subsecondDigits which would allow for future arbitrarty precision. This would allow us to go for micro or even nanosecond precision in future.

@apaprocki
Copy link

future arbitrarty precision

Also worth noting that we already have a (quasi-)polyfilled use case where we add getMicroseconds() / getUTCMicroseconds() accessors to standard Date objects. Seconding that limiting to milliseconds is not desirable.

@FrankYFTang
Copy link
Contributor Author

I am going to revise this PR to change the option name to 'subsecond', should the 'type' of the formatToParts also named 'subsecond' instead of 'fractionalSecond'?

@apaprocki
Copy link

FWIW, in our C++ code we refer to it as fractionalSecondPrecision documented to be the number of decimal places used for seconds. If you name it that, then it would make sense to keep the type as fractionalSecond. What do you think @pipobscure?

Constellation added a commit to Constellation/test262 that referenced this pull request Aug 26, 2020
…ZoneName

In this PR[1], fractionalSecondDigits is listed earlier than timeZoneName in table 6[2].
So, accessing order of fractionalSecondDigits in [3]'s step-29 should be earlier than timeZoneName.

[1]: tc39/ecma402#347
[2]: https://tc39.es/ecma402/#sec-datetimeformat-abstracts
[3]: https://tc39.es/ecma402/#sec-initializedatetimeformat
Constellation added a commit to Constellation/test262 that referenced this pull request Aug 26, 2020
…ZoneName

In this PR[1], fractionalSecondDigits is listed earlier than timeZoneName in table 6[2].
So, accessing order of fractionalSecondDigits in [3]'s step-29 should be earlier than timeZoneName.

[1]: tc39/ecma402#347
[2]: https://tc39.es/ecma402/#sec-datetimeformat-abstracts
[3]: https://tc39.es/ecma402/#sec-initializedatetimeformat
rwaldron pushed a commit to tc39/test262 that referenced this pull request Sep 2, 2020
…ZoneName

In this PR[1], fractionalSecondDigits is listed earlier than timeZoneName in table 6[2].
So, accessing order of fractionalSecondDigits in [3]'s step-29 should be earlier than timeZoneName.

[1]: tc39/ecma402#347
[2]: https://tc39.es/ecma402/#sec-datetimeformat-abstracts
[3]: https://tc39.es/ecma402/#sec-initializedatetimeformat
@exikyut
Copy link

exikyut commented Sep 13, 2020

Hi! I was digging around for ways to format a date, and stumbled on DateTimeFormat. Kind of amazing to realize this only been out a few weeks/months (StackOverflow hasn't noticed it at all yet!), and I'm already able to leverage it. :)

Given thread recency/activity, existing mentions of implementation behavior, and the obvious benefits of skipping the triage lag at bugs.chromium.org...

The following "works" (without behaving as intended) in Chromium 83, and fails in Chrome 85.

new Intl.DateTimeFormat('en', { fractionalSecondDigits: 2, dateStyle: 'medium', timeStyle: 'short' }).format();

Chrome 83: "Sep 13, 2020, 4:11 PM"

Chrome 85:

VM58:1 Uncaught TypeError: Invalid option : dateStyle
    at new DateTimeFormat (<anonymous>)
    at <anonymous>:1:1

It appears that this is because Chrome ~85 has just landed actual support for fractional seconds, and the code now does the relevant bounds checking. The error message is somewhat confusing though.

As side meta-discussion, I'm curious how I'd achieve "Sep 13, 2020, 4:11.267 PM". It appears not currently possible to do this by just using timeStyle. Perhaps that's an interesting use case.

@sffc
Copy link
Contributor

sffc commented Sep 14, 2020

@exikyut

The exception in Chrome 85 is due to a spec change in dateStyle/timeStyle, which were still draft at the time (they've since become stable). See https://bugs.chromium.org/p/chromium/issues/detail?id=1124778. This spec change was done in part to avoid the surprising behavior you noted.

In order to get your desired format, you just need to avoid using dateStyle/timeStyle.

new Intl.DateTimeFormat('en', {
  year: "numeric",
  month: "short",
  day: "numeric",
  hour: "numeric",
  minute: "2-digit",
  second: "2-digit",
  fractionalSecondDigits: 2,
}).format();

@FrankYFTang
Copy link
Contributor Author

FrankYFTang commented Sep 14, 2020

The error message is somewhat confusing though.

Agree. And that is fixed in chrome on "Fri, Sep 11, 2020, 4:28 AM PDT" in
https://chromium-review.googlesource.com/c/v8/v8/+/2400598
for https://bugs.chromium.org/p/v8/issues/detail?id=10880
and it now should match the error message in FireFox 80 except we use upper case "Can't" but FireFox 80 use lowercase "can't"

The new error message you will see in Chrome 87 is

d8> new Intl.DateTimeFormat('en', { fractionalSecondDigits: 2, dateStyle: 'medium', timeStyle: 'short' }).format();
(d8):1: TypeError: Can't set option fractionalSecondDigits when dateStyle is used
new Intl.DateTimeFormat('en', { fractionalSecondDigits: 2, dateStyle: 'medium', timeStyle: 'short' }).format();
^
TypeError: Can't set option fractionalSecondDigits when dateStyle is used
    at new DateTimeFormat (<anonymous>)
    at (d8):1:1

@FrankYFTang
Copy link
Contributor Author

What is blocking this PR to be merged?

@leobalter
Copy link
Member

@FrankYFTang yesterday I did some review work on pending PRs and jumped those with the needs tests labels. Please let me take a look again here.

@leobalter
Copy link
Member

I'm afraid we never recorded consensus in this PR thread, please correct me if I'm wrong. I'll go through the meeting notes and confirm.

@leobalter leobalter added has consensus Has consensus from TC39-TG2 has consensus (TG1) Has consensus from TC39-TG1 and removed needs tests labels Jan 5, 2021
@leobalter leobalter merged commit a6a16fa into master Jan 5, 2021
@leobalter leobalter deleted the FrankYFTang-patch-3 branch January 5, 2021 21:06
@sffc
Copy link
Contributor

sffc commented Jan 5, 2021

Consensus is recorded in the wiki with a link to the notes from 2019-07-11:

https://github.com/tc39/ecma402/blob/master/meetings/notes-2019-07-11.md#normative-add-fractionalseconddigits-option

@leobalter
Copy link
Member

Yes, the notes helped me a lot and, as I said, I quickly jumped over this PR due to the existing labels.

1. Else if _p_ matches a Property column of the row in <emu-xref href="#table-datetimeformat-components"></emu-xref>, then
1. Let _f_ be the value of _dateTimeFormat_'s internal slot whose name is the Internal Slot column of the matching row.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The removal of this line (line 336) is a mistake during editing. Please add it back @leobalter Totally my fault.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c: datetime Component: dates, times, timezones enhancement has consensus (TG1) Has consensus from TC39-TG1 has consensus Has consensus from TC39-TG2 s: in progress Status: the issue has an active proposal Small Smaller change solvable in a Pull Request
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.