MC-21331 Intl.DateTimeFormat returns different format

[Sakai-san]

Follow-up of PR merged on the 2021-04-12.

[neonnoon]

I'm not sure I understood this right. Is it dependent on the user's settings or rather on the user's runtime, i.e. browser?
The issue in #6 was that the runtime (here node) did not support it. Modern browser so, see: https://caniuse.com/?search=Intl.DateTimeFormat

[sosostris]

I'm not sure I understood this right. Is it dependent on the user's settings or rather on the user's runtime, i.e. browser?
The issue in #6 was that the runtime (here node) did not support it. Modern browser so, see: https://caniuse.com/?search=Intl.DateTimeFormat

Yeah the issue seems quite tricky 🎃 I tried it in my browser and the format looks good, but the test is failing in our local dev environment and in the build environment, e.g. 2021.04.16 is displayed as 2021/04/16 🐙 "/" instead of "."

I'm not sure if it's sth that we only need to fix for the test, or sth that might potentially result in different display format for users with different user_setting 🤔 (e.g. an old windows machine with edge)
Would be good if we could conclude that it's only a test thing.

I have put this issue as an item in our next sync meeting 😎 and @Sakai-san is currently looking a bit into it 😇

[neonnoon]

Did you see #6 (comment) and the issue linked there: nodejs/node#19214

Node <= 12 does not have full ICU support, i.e., it does not come with german date formats. I am guessing you're using node <= 12 on the build hosts. Upgrading node to 14 (LTS) should fix the tests.

Am I the only one confused with a lot of PRs issues etc. covering the same topic? Or do you have some system I just don't understand?

[sosostris]

Am I the only one confused with a lot of PRs issues etc. covering the same topic? Or do you have some system I just don't understand?
Can you elaborate why you prefer this tracked in a TODO in the code rather than a Jira or github issue?

Sorry @neonnoon @Sakai-san for the confusion! Actually I got confused myself too (still getting used to Github 😂 ), but now I think I get everything and let's see if this helps clears up confusion ;)

• There are only two PRs covering the topic of TRS timezone display: PR6 and this one
• PR6 is merged and let's leave that one and keep the discussion here ;)
• This current PR is mapped to an internal Jira ticket and addresses the issue that internal tests were breaking
• The "TODO" will be removed soon, but agree that it's best not to track things with "TODO"
• Regarding the fixing of the broken tests, #6 (comment) could definitely fix the tests, but there is some concern that, suppose an external user is using Pyrene to develop his/her own application, we could not predict what will be the node version in that environment, would it lead to format display issues? If this would not a potential issue, we would go with this fixing also internally; but if this could potentially turn into some inconsistency in display, we might find a better solution? (But on the other hand, maybe it's fine as long as for a user the display format is consistent, be it 2021/12/21 or 2021.12.21 😂 )

[Sakai-san]

@neonnoon Thanks for your comments. Let me explain what this PR is about in two words.
You used the internationalization built-in DateTimeFormat for formatting a date here, which was very good :-)

So :

• the date is formatted depending on the user language in browsers.
• the date is formatted depending on the nodejs version on servers (like Continous Integration pipelines).

The thing is that we want a universal formatting. Means the same whatever language setting or nodejs version.

[neonnoon]

I think there's a misunderstanding between available Intl languages and the browser language.

The de here:

pyrene/pyrene/src/components/TimeRangeSelector/TimeRangeNavigationBar/TimeRangeNavigationBar.jsx

Line 31 in 0eedd32

const locale = new Intl.DateTimeFormat('de', {

specifically uses german locale, independent from the browser's language setting. As long as the browser supports german it will display it correctly in german style formatting. And this is independent of the user settings.

It is quite difficult to figure out a list of "guaranteed" supported languages. From what I could gather, most browsers are based on http://site.icu-project.org/home (as is node), which contains german.

Now the issue was that full-icu was not contained in node prior to 13. So node 12 does not know what to do with de.

So we're not talking about selected browser languages, but about what browsers fully support Intl (all modern ones), and which nodes fully support Intl (>= 13).

So, we could add this to package.json to indicate that we need node >=13: { "engines" : { "node" : ">=13" } }

Does that make sense? Or did I misunderstand your comments?

[neonnoon]

For example, try new Intl.DateTimeFormat('ja-JP').format(new Date()) in your console, even assuming that neither your OS nor your browser are set to Japanese, you'll get "2021/4/17".

---

@neonnoon Thanks a lot for your comprehensive explanation. You were right and I was actually wrong.

To sum up, there are two solutions for fixing this language support issue that we are facing with our Continuous Deployment within our main project.

1. Your solution, means to force a minimum nodejs version in pacakge.json of Pyrene.
2. My solution, means to use an external library for computing the local time. That library is date-fns-tz that we already use in development mode, as you know.

I tried both solution and prefer yours. Why ? Because long term it means getting rid of external dependencies also in development mode.

But the problem is the following. We are using nodejs v12.13.0 for the main project. As you also know, Pyrene is just a dependency within a sub-project of that main project. Changing the nodejs version in the main config also affects the npm version. Doing so creates new issues with other npm nodules in other sub-projects.

To conclude, I would say we can go for a pragmatic solution this time. However long term speaking I ll try to push built-in solutions like the Intl object you used so we could remove npm dependencies.

[neonnoon]

Huh, I'm confused, why is your comment posted under my name? 😳😀 Did you use Edit rather that Quote?

Anyway, I agree with your suggested options (I guess the initial use of date-fns-timezone wasn't right, but your simplified use of it is correct, nice!).

I do not have a strong preference for either of the two solutions. Whatever is easier for you (also short time).
If you prefer to go for the date-fns-timezone options, we might want to add node 12 back to the workflow config so that there's better consistency between internal and public CI?

Either way though, we may wanna think about node 14 and npm 7 eventually.

[Sakai-san]

Huh, I'm confused, why is your comment posted under my name? 😳😀 Did you use Edit rather that Quote?

Anyway, I agree with your suggested options (I guess the initial use of date-fns-timezone wasn't right, but your simplified use of it is correct, nice!).

I do not have a strong preference for either of the two solutions. Whatever is easier for you (also short time).
If you prefer to go for the date-fns-timezone options, we might want to add node 12 back to the workflow config so that there's better consistency between internal and public CI?

Either way though, we may wanna think about node 14 and npm 7 eventually.

@neonnoon Correct. Sorry, I ll quote from now on. Ok let's do so for now. I am not sure to understand your sentence

we might want to add node 12 back to the workflow config so that there's better consistency between internal and public CI?

You'd like to add that config to the package.json ? I can do it in the present PR if you'd like to ?

{ "engines" : { "node" : ">=12 <13.0.0"} }

[neonnoon]

The engines part I would only add if there is a strict requirement why only certain versions are supported. This is the case if we require node >= 14 for full ICU support.

What I meant to refer to was this change:
https://github.com/open-ch/pyrene/pull/6/files#diff-107e910e9f2ebfb9a741fa10b2aa7100cc1fc4f5f3aca2dfe78b905cbd73c0d2L14

If you keep using node 12 for your internal CI environment, it probably makes sense to revert the version used by github actions.

So, to summarise, the way I see it: There are two options:

1. Use Intl.DateTimeFormat

• requires node >= 14
  • should be added to engine in package.json
  • needs to be updated in internal CI environment

2. Use date-fns-timezone

• you might want to downgrade github action's node-version to 12 as well

I do not have a strong opinion for any of those decisions (just wanted to make sure we don't misunderstand each other on the options). So I suggest you discuss and decide this internally?

[sonarcloud]

Kudos, SonarCloud Quality Gate passed!

0 Bugs
0 Vulnerabilities
0 Security Hotspots
0 Code Smells

No Coverage information
0.0% Duplication

[Sakai-san]

@neonnoon Thank you very much for sharing your expertise as well as your time. It was indeed super helpful. If you agree we I will now submit the change we came up within the present PR to the team. Then if the team agrees we can merge this PR today. Would be nice if you could also accept or reject the PR. Thanks again.

[sosostris]

Thank you so much @neonnoon @Sakai-san for the thorough discussions! I learned a lot from you :) 🤩 🙏

I agree that for now it might be better to go with solution 2 because of the different node environments in the projects:

Use date-fns-timezone
you might want to downgrade github action's node-version to 12 as well

But since we're abandoning internal CI - it's already announced internally that further new PRs should all be targeting at Github directly, and like @neonnoon mentioned, in general it's nice to think about node 14 and npm 7, could we keep the current node-version in Github action?
-> Ah, I see that the node version has already be downgraded to 12, kk np :) it is good to align it with the node environment we have internally!

So possible action items could be:

• merge this PR
• do a incremental release directly in Github without involving local repo (especially the tagged commit locally)
• formulate a plan for node 14 / npm 7 (have added as item to discuss for next sync meeting 😉 )

[sosostris]

Thank you very much for the PR! I learned a lot!