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 FrankYFTang commented May 21, 2019

@FrankYFTang FrankYFTang requested review from leobalter, caridy, littledan, anba and sffc May 21, 2019
@FrankYFTang
Copy link
Contributor Author

@FrankYFTang FrankYFTang commented 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

@HolgerJeromin HolgerJeromin 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)?

@FrankYFTang
Copy link
Contributor Author

@FrankYFTang 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

@FrankYFTang FrankYFTang commented May 22, 2019

@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

This comment has been minimized.

@FrankYFTang

FrankYFTang May 22, 2019
Author Contributor

@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?

This comment has been minimized.

@spectranaut

spectranaut May 23, 2019
Contributor

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]].

This comment has been minimized.

@FrankYFTang

FrankYFTang May 23, 2019
Author Contributor

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

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

This comment has been minimized.

@HolgerJeromin

HolgerJeromin May 24, 2019

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?

This comment has been minimized.

@FrankYFTang

FrankYFTang May 26, 2019
Author Contributor

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).

@FrankYFTang FrankYFTang requested a review from zbraniecki May 22, 2019
@HolgerJeromin
Copy link

@HolgerJeromin HolgerJeromin commented May 23, 2019

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"
spec/datetimeformat.html Outdated Show resolved Hide resolved
@FrankYFTang

This comment was marked as off-topic.

@leobalter

This comment was marked as off-topic.

@pipobscure
Copy link

@pipobscure 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

@apaprocki apaprocki commented Jun 5, 2019

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

@FrankYFTang FrankYFTang commented Jun 5, 2019

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

@apaprocki apaprocki commented Jun 5, 2019

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
pull bot pushed a commit to Qwerty0x64/v8 that referenced this pull request Aug 27, 2020
Move fractionalSecondsDigits between second and timeZoneName
Change order of reading options.
To sync with the July 20 PR change in
tc39/ecma402@ba085a9
Latest ECMA402 PR tc39/ecma402#347

Bug: v8:10836
Change-Id: Ia414e0c7cc18502ccabaf02abd19861410b87cae
Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/2378460
Reviewed-by: Jakob Kummerow <jkummerow@chromium.org>
Commit-Queue: Frank Tang <ftang@chromium.org>
Cr-Commit-Position: refs/heads/master@{#69591}
rwaldron added 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
@sffc sffc moved this from Previously Discussed to Priority Issues in ECMA-402 Meeting Topics Sep 10, 2020
@exikyut
Copy link

@exikyut 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
Collaborator

@sffc 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 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

@FrankYFTang FrankYFTang commented Jan 5, 2021

What is blocking this PR to be merged?

@leobalter
Copy link
Member

@leobalter leobalter commented Jan 5, 2021

@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

@leobalter leobalter commented Jan 5, 2021

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 merged commit a6a16fa into master Jan 5, 2021
0 of 2 checks passed
0 of 2 checks passed
continuous-integration/travis-ci/pr The Travis CI build is in progress
Details
continuous-integration/travis-ci/push The Travis CI build is in progress
Details
@leobalter leobalter deleted the FrankYFTang-patch-3 branch Jan 5, 2021
@sffc
Copy link
Collaborator

@sffc 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

@leobalter leobalter commented Jan 5, 2021

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.

This comment has been minimized.

@FrankYFTang

FrankYFTang Jan 11, 2021
Author Contributor

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
Projects
ECMA-402 Meeting Topics
Previously Discussed
Linked issues

Successfully merging this pull request may close these issues.

None yet