Update Date.toLocaleString (and related) for Nodejs

[GitTom]

Nodejs' support for locale's has been "build" dependent with the canonical build (that distributed from https://nodejs.org/en/download/) not including locale data, so Date.toLocaleString() and related functions ignore their parameters. I downloaded the default (64-bit) v13.0.1 for Windows from nodejs.org and tested samples of the locale, options, and IANA tz names and found that they worked as expected, where they were ignored on v12.13. Note that v13 is now the 'current' version.

I will make note of this PR on an appropriate issue of the nodejs repo and invite others to comment here.

[edit: just remembered that I installed v13 using nvm-windows, not by downloading directly from nodejs.org.]

[srl295]

ignore their parameters

Just to be precise, they don't actually ignore their parameters— the parameters have been supported by default since Node v0.12. However, only English (en) related data was available by default.

Example, Node.js 12:

> new Date().toLocaleString([], {month: 'long'})
'October'

I see in https://github.com/mdn/browser-compat-data/blob/master/javascript/builtins/String.json#L396 that there's support for a little flag. Perhaps these can be added (Date.toLocaleString) as v0.12 for example, with a flag noting that extra data was required until 13?

[GitTom]

@srl295 Ok, thanks for the clarification.

I created another PR to add v13 to the list of Nodejs releases. It will need to be merged before this PR can pass.

[queengooborg]

Now that the data for NodeJS v13 has been added, may we rebase this so we can re-run a linter test?

[thom4parisot]

Well, it worked for earlier releases of Node (since Node v8.x as far as I remember), but you had to compile from sources to include ICU data other than for English language.

Does it count as "behind a flag", like some other compatibility options do?

If so, I'm happy to find my way to add these extra bits as an additional PR.

[thom4parisot]

So in that matter, there is the NODE_ICU_DATA environment variable and the --icu-data-dir node runtime option to specify an ICU data file location. If combined with the full-icu npm package, these data can be installed as part of the npm install project setup step.

So to recap my understanding, from Node v13 users can use Intl with other formats/languages than English by default (what is proposed by this PR) — since Node v0.11/v0.12, they had to specify ICU data location at runtime via the aforementioned variable/option, unless embedded/linked to the OS at compile time (what I propose to clarify in an additional PR).

It's only useful and more correct for the folks who would wonder how to deal with the Intl API in Node without upgrading, when browsing MDN (and other projects which rely on browser-compat-data).

[longlho]

Yeah I don't think this is accurate. All Intl APIs availabilities in MDN don't assume all locales supported. Node 0.12 support for toLocaleString should be accurate.

browser-compat-data is not designed to handle this use case. Even when you see Intl.NumberFormat being available in IE11 and such, it doesn't not include languages like cy and that's implementation details in the spec so users should not assume their locale is supported.

[GitTom]

Just a clarification about it working for "English (en) related data" prior to Node 13... it only worked for "en-us" - none of the other English speaking countries using American formats so it produced incorrect results for them too. And potentially ambiguously incorrect formats (the date format) so it is an important distinction.

[longlho]

yeah I agree that it's ambiguous but I'm not sure if changing it to Node 13 in MDN is the right solution. As I said this changes implementation to implementation and potentially version to version within an implementation.

[GitTom]

@longlho sure, minor changes to the locale data will continue, but that is quite different than going from supporting just 1 country to supporting all countries.

And note that I'm not proposing to list the v13 requirement for the functions, just the parameters.

I think an argument could be made, though, that specifying the v13 requirement for only for the locale parameter would be sufficient, along with a flag/comment explaining that it uses US English regardless of the input?

The ideal solution would be for node to produce an error when a locale other then 'en-US' is specified. The fact that it silently ignores the locale and, for most countries, produces dates that look correct but are actually wrong (depending on the day of the month, of course), makes me want to error on the side of the user avoiding these API's until v13.

[longlho]

Returning an Error would be wrong. The spec indicates that locale negotiation is best effort, and can fallback to DefaultLocale which is host-dependent.

See https://www.ecma-international.org/ecma-402/6.0/index.html#sec-internal-slots

Again, an implementation can ship with 1 locale and would still be spec-compliant, which is what MDN captures. It does not capture implementation details that are un-spec'ed.

[GitTom]

Ok, I accept your point that it is spec-compliant, but I think that point is separate from what should be communicated in the compatibility section to help devs avoid mistakes and misunderstandings.

I also see that these functions will in fact throw an exception if you pass them an unsupported language, but not when you pass them an unsupported locale. That seems like a poor decision since the many English locales other then en-US will all succeed but produce erroneous results, but Yes, the docs say 'language' and not 'locale' and I missed that.

This all leads me back to what I was suggesting in my last comment: that we specify the 'locale' parameter as requiring node 13 but not the rest. I still feel that the locale parameter can't be listed as supported when the function ignores the specified locale, regardless of whether that is spec-compliant behavior.

BTW, is there a way to get a list of supported locales, or otherwise test whether a locale is supported (where the locale in question is only known at runtime)?

[longlho]

Yes there’s a supportedLocalesOf on Intl APIs like DateTimeFormat

…

On Wed, Oct 30, 2019 at 11:25 PM Tom ***@***.***> wrote: Ok, I accept your point that it is spec-compliant, but I think that point is separate from what should be communicated in the compatibility section to help devs avoid mistakes and misunderstandings. I also see that these functions will in fact throw an exception if you pass them an unsupported language, but not when you pass them an unsupported locale. That seems like a poor decision since the many English locales other then en-US will all succeed but produce erroneous results, but Yes, the docs say 'language' and not 'locale' and I missed that. This all leads me back to what I was suggesting in my last comment: that we specify the 'locale' parameter as requiring node 13 but not the rest. I still feel that the locale parameter can't be listed as supported when the function ignores the specified locale, regardless of whether that is spec-compliant behavior. BTW, is there a way to get a list of supported locales, or otherwise test whether a locale is supported (where the locale in question is only known at runtime)? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#5068?email_source=notifications&email_token=AABQM34WXVFPX3QRA5K4JRDQRJFY7A5CNFSM4JGQEYV2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOECWOHCI#issuecomment-548201353>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AABQM344QBU4FTQRNHXOYOTQRJFY7ANCNFSM4JGQEYVQ> .

[srl295]

• Intl support landed on by default in Node v0.12, 2015-02-06, not v0.11. edit but, looks like it got backported at some point- I forgot about that.

[GitTom]

Yikes, the supportedLocalesOf API rather exacerbates the issue that has got me all hot and bothered.

It checks the language but ignores the country part of the locale. So, for the 15 English speaking countries, it gets the date format wrong for 14 of them, and ambiguously wrong (the part that really concerns me) for 12 of them. Sure, some of these countries are small, but the 12 for which it is a more serious problem include India and Britain so it is significant.

Maybe I should open an issue on the Node repo about this. I'm not saying it is a bug, but I think it is worth noting as a serious problem.

Am I missing something?

I will create a separate comment about my plan for this PR when I get a chance.

[longlho]

Hmm what did you specifically test?

[GitTom]

Run the following code in Node < 13, and note that it believes that it believes that all the locales are supported, but gives the US date format for them all.

const locales = [
  'en-AU', 'en-BZ', 'en-CA', 'en-GB', 'en-IE', 'en-IN', 'en-JM', 'en-MY',
  'en-NZ', 'en-PH', 'en-SG', 'en-TT', 'en-US', 'en-ZA', 'en-ZW'
];
const slo = Intl.DateTimeFormat.supportedLocalesOf( locales, { localeMatcher: 'lookup' } );
console.log( `Supported locales = ${slo.join(', ')}.` );

const d = new Date();
locales.forEach( (loc) => { console.log( `In '${loc}' the date is ${d.toLocaleString( loc )}.` ); } );

[longlho]

That is expected, and it falls back to en.

[srl295]

Maybe I should open an issue on the Node repo about this. I'm not saying it is a bug, but I think it is worth noting as a serious problem.

Yes, it sounds like this is more of a node question than an MDN question.

From an implementation perspective, the data provided is en (which == en-US). I can't speak to supportedLocalesOf off the top of my head, I'm not sure how it's implemented.

In any event, it's all the more reason NOT to use the 'small' data. Use full-icu or upgrade to v13.

This all leads me back to what I was suggesting in my last comment: that we specify the 'locale' parameter as requiring node 13 but not the rest. I still feel that the locale parameter can't be listed as supported when the function ignores the specified locale, regardless of whether that is spec-compliant behavior.

But it doesn't ignore the specified locale. Nothing changed in v13 in this regard. So it's misleading to list this as a v13 feature. An example suffices:

$ nvm i 0.12
v0.12.18 is already installed.
Now using node v0.12.18 (npm v2.15.11)
$ node
> new Date().toLocaleString("en-u-ca-hebrew-nu-thai")
'๓ Heshvan ๕๗๘๐, ๑๐:๒๒:๕๕ AM'

Is a "flag" not an option? since v0.12, but see important note about data?

[queengooborg]

Overall, it sounds like these features were implemented all the way back in Node v0.12, but there were issues with the language support until Node 13, correct? If that’s the case, we should set “0.12” as the version_added and add a notes value explaining this behavior. From what it sounds like, just having English as a language is fully spec-compliant, but this issue is significant to record in BCD.

[longlho]

That's not entirely accurate either because you can compile Node 12- with full-icu, which is what a lot (AFAIK, Yahoo & Dropbox) of tech companies run in production, and that has all the locales. All in all, it's a specific platform implementation detail that I'm not sure should be captured here.

[queengooborg]

@longlho Just to let you know, features that must be enabled by recompiling Node are not within the scope of BCD. If it were a runtime flag like node --with-full-icu, then yes, but our data doesn't focus on features not compiled into official release versions of browsers/NodeJS.

[GitTom]

Ok, I intend to update this PR to the following:

For the functions
Date.toLocaleString
Date.toLocaleDateString
Date.toLocaleTimeString

I will make the following changes to the data.

• For the fn itself: true
• IANA time zone names: true
• options: true
• locales: 13.0.0
  I believe I need to specify "13.0.0" to match what I added here where I used the full version string for consistency, though this may look odd in the compatibility table.

And I will add an "implementation note" (ie. using 'notes' property in the data) to 'locales' about it only supporting "en-US" prior to v13.

My intention was just to do the Date.* functions as a first step, and leave all of Intl for now, but the issue I raised in the comments above make me think that it is important that I add an "implementation note" to Intl.DateTimeFormat.supportedLocalesOf() as well. It currently has no data. I will set it to 'true' and add an implementation note to warn that it ignores the country portion of the locale.

[longlho]

Hmm again, supportedLocalesOf does not ignore country. Certain implementations may choose too. The locale lookup hierarchy is more complicated than just the language tag (e.g zh-CN is a legacy alias to zh-Hant-CN and such)

[GitTom]

No, I wrote that it ignores the country portion, not the language portion.

[queengooborg]

@GitTom It’s fine, we don’t really have any rules about that in this repository. :)

[GitTom]

I returning to the central issue. I'm confused about the argument you were making in support of your assertion that 'locale' should be marked 'true' (I believe that is what you are saying).

Initially it seemed that you were making an assertion about Node, but then you wrote "I said Node uses V8 JS engine, which may/may not come w/ full ICU data.". The point I'm making is that it doesn't, until v13. I'm not clear whether you are contesting that?

And if it doesn't - as I contend - then it always uses the 'en-US' locale, regardless of what is specified in the locale parameter. I'm not clear on why we would want to tell developers that it the parameter is supported if the behavior is always the same regardless of the value of the parameter.

Unless they happen to specify 'en-US', they value they get back is incorrect. I'm not saying it violates the spec - that is not the issue - I'm saying it is wrong. They will get a different response - the correct response - starting with version 13.

[longlho]

I don't think it's a valid argument to say it is wrong. Keep in mind every ICU dist comes with a specific CLDR data version and that also changes. So Node 13.0.0 gets packaged with 1 specific CLDR data version. I can argue that anything that's not latest will produce wrong output in your scenario. For example, CLDR 63/64 (I can't remember) changes the output of Intl.RelativeTimeFormat.format(0, 'hour') from 0 hour ago to this hour. Which one is correct then?

Also, are you going to make a note saying Chrome doesn't all languages such as cy?

CLDR data comes from survey so it is also highly debatable on correctness.

[GitTom]

Ok, we are not going to agree, so I think we need some arbitration from MDN here, please?

Otherwise I suggest we close the PR and make no change to the data. There is no point in us arguing back and forth any further.

I thought this was straight-forward so either I'm missing something, or I'm explaining it poorly - so I'll try to restate it as clearly as I can...

This is about the 'locale' parameter in several functions date formatting functions in the official Node for version below 13.

Those implementations do not use the value provided for that parameter - they use 'en-us' instead. There are 3 possible outcomes when the caller specifies a value for 'locale'... either the value is silently ignored and the functions produces the wrong result, or the functions throws an exception, or it produces the correct result if the value specified for the parameter matches the value that the implementation uses by coincidence (as-in the saying 'a clock is right twice a day').

We seem to have a different understanding of what 'wrong' means in this case, so... I mean that the functions in question will produce eg. "11/03/19" instead of "03/11/19". It is this understanding of 'wrong' that is relevant to the developers who will be reading this documentation. (I'm not saying anything about whether these implementations and behaviors conform to the spec.)

Note that the issue being debated is how to treat the 'locale' parameter. I maintain that it should be marked as v13 for Node, as opposed to marking it as simply supported ('true' in the data).
[edit]

[srl295]

OK like to summarize some things..

• My overall recommendation on this PR: something like " 'locale' is supported since v0.12, but for v < 13: see docs on configuring data". 62k downloads a week simply install the "full-icu" module to accomplish this, no recompiling needed.

• if there are concerns about how en-* (where * ≠ US ) is handled in node under the now-historic small-icu settings, please file a separate issue to track that in the node repo. I probably should have included en-* instead of just en, my old bug. Nothing intentional against non-USian English speakers, any more than the whole problem in the first place of the less-than-global English-only compromise. Call it "US-only by mistake". @GitTom it is most definitely not ignoring the locale parameter, it is just that the user may not have configured all datasets. I already showed how v0.12.0 with no additional data or recompiling supports the locale parameter per spec.

• @longlho :

CLDR data comes from survey so it is also highly debatable on correctness.

This isn't the right place to discuss this, but it's actually not an accurate statement about the process or result, but please write/mention me directly if you would like to discuss further how this works! edit my DM open on Twitter, or whatever. let's chat…

[queengooborg]

It's true, there's a lot of disagreement in this PR, unfortunately. I would also like to mention that, at the moment, NodeJS is not a high priority in BCD; our main focus is the Chrome and Safari JavaScript data.

I do not feel that setting the features to NodeJS v13 is the right call here. The locales themselves may not have been supported until NodeJS v13, but I would say that support is "is the function implemented based upon the spec"? Reading through the comments on this PR, it sounds like they have been supported since NodeJS v8, perhaps even all the way back to v0.12 (which checks out with the Chrome data) -- and that is what the version number should be in BCD. And then we can add a note saying Until NodeJS v13, only the English locale was available; however, a custom ICU data path could be specified through the <code>--icu-data-dir=...</code> flag or the <code>NODE_ICU_DATA</code> environment variable.

[GitTom]

Yes, I am in full agreement with what @srl295 has suggested.

(I was editing my summary and changing the wording regarding 'ignoring' as you wrote your response.)

[longlho]

@srl295 sry about that I'm definitely oversimplifying the process.

I agree w/ you & @vinyldarkscratch on the resolution.

[srl295]

@longlho no problem. If it was just a survey we'd have continuous releases… maybe hourly! edit great, now i'll have nightmares about a CI process where someone votes to change something and it cuts a new node.js release :D [just because it would be technically possible doesn't mean it's a good idea…]

[GitTom]

I appreciate @srl295 and @vinyldarkscratch weighing in, but I'm not clear if there is consensus.

I think @srl295 was supporting my proposal that the functions themselves would simply be marked as supported, but the 'locale' parameter would be v13 with a note (as opposed to marking 'locale' as supported all along like the rest which I did not agree with).

@vinyldarkscratch - it sounds like you intended to demur regarding my proposal but then I'm not sure... Let me clarify that the idea of marking the function itself as v13 was dropped quite quickly and what we've been arguing about is how the 'locale' parameter would be treated. (Which feature are you suggesting would be marked as supported since v8?)

[GitTom]

Ok, I made the changes that we've discussed. The note text is rather long, but I think that the way these notes are handled on the doc site is good, allowing for clear notes without polluting the page. The warning in parenthesis about the date format being wrong only is only on the date (vs. time) functions.

"nodejs": {
    "version_added": "0.12",
    "notes": "Prior to Nodejs 13 only the locale data for 'en-us' is available, by default.  When other English locales are specified, the function silently falls back to 'en-us' (so date formats will be incorrect but may appear correct).  When other languages are specified, it throws a RangeError.  In order to make full ICU (locale) data available for versions prior to 13, see docs on '--with-full-icu=' flag and how to provide the data."
},

I know that there are more related changes to be made but since we had a hard time agreeing I stuck to my original scope.

Thanks for all input!

[queengooborg]

I'd say this is looking great, thanks for the PR! I just have a few changes I'd like to have made to the notes for grammar and formatting. (I also believe that the part about the date format being wrong doesn't need to be specified, as it's implied by the silent fallback to en-us.)

Once the notes are updated, I'd say this is ready for merging!

[queengooborg]

Looking good to me, thank you for the PR! Let's get this merged!

[GitTom]

@vinyldarkscratch yes, the point about the date formatting being wrong was there for the "but may appear correct" part, which is an unusual gotcha that will make people think it is working when it isn't. I'm from Canada where our dates are equal portion US format and UK/Commonwealth format, so we know how bad this ambiguity problem can be (hence our gov' is pushing hard to ISO format).

But I accepted your changes and hope we can close this as soon as possible! Thanks.

[srl295]

🎉