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

attempt getting rid date-fns-tz in prod build #32

Closed
wants to merge 7 commits into from

Conversation

Sakai-san
Copy link
Contributor

@Sakai-san Sakai-san commented Apr 21, 2021

Using @neonnoon approach for converting a date with time zone A to date with time zone B, using built-in JavaScript utility was a better approach then import third-party lib.

@Sakai-san Sakai-san changed the title attempt gettingv rid date-fns-tz attempt gettingv rid date-fns-tz - WIP Apr 21, 2021
@Sakai-san Sakai-san changed the title attempt gettingv rid date-fns-tz - WIP attempt getting rid date-fns-tz - WIP Apr 22, 2021
@Sakai-san Sakai-san changed the title attempt getting rid date-fns-tz - WIP attempt getting rid date-fns-tz in prod bundle - WIP Apr 22, 2021
@sonarcloud
Copy link

sonarcloud bot commented Apr 22, 2021

Kudos, SonarCloud Quality Gate passed!

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
0.0% 0.0% Duplication

@Sakai-san Sakai-san requested a review from neonnoon April 22, 2021 00:34
@Sakai-san Sakai-san changed the title attempt getting rid date-fns-tz in prod bundle - WIP attempt getting rid date-fns-tz in prod bundle Apr 22, 2021
@Sakai-san Sakai-san changed the title attempt getting rid date-fns-tz in prod bundle attempt getting rid date-fns-tz in prod build Apr 22, 2021
@neonnoon
Copy link
Contributor

Can you elaborate why you prefer going for this variant? Basically you're saying you're trusting browsers to implement en-us properly (so you can use it to parse and build the manual date), but not de?

If the only reason is not having node 14 available on the (to be deprecated) internal CI, I don't quite see the point.

Anyway, I think I am missing some context of internal discussions you're having, from my point of view we're going a bit in circles.

@Sakai-san
Copy link
Contributor Author

Sakai-san commented Apr 22, 2021

Anyway, I think I am missing some context of internal discussions you're having, from my point of view we're going a bit in circles.

Hi @neonnoon. Ok let me give some context. It relates to the a recent merged PR actually.
The basic idea is to not use third party library while built-in JS object do the same job (like you did in the past).

The language de, en-US does not matter much. Indeed we do have our own format anyway, namely 'dd.MM.yyyy, HH:mm'.

The only problem with de is that it seems not to be in node 12. What is important is that the built-in JS feature convert the date into different time zone (including summer time) and thus we can skip importing date-fns-tz.

The present PR is nothing but a proposal. If there no need I ll close it. All good.

@neonnoon
Copy link
Contributor

The basic idea is to not use third party library while built-in JS object do the same job (like you did in the past).

Exactly, but then we reverted that. And now we're reverting the revert. Which is why I think we're going in circles. 😄

The language de, en-US does not matter much. Indeed we do have our own format anyway, namely 'dd.MM.yyyy, HH:mm'.

'dd.MM.yyyy, HH:mm' is actually de, just without seconds.

Again, no strong opinion either way. But I think we should come to a conclusion and an end with this topic eventually.

@Sakai-san Sakai-san closed this Apr 22, 2021
@Sakai-san Sakai-san deleted the attempt-getting-rid-date-fns-tz branch April 22, 2021 08:40
@Sakai-san
Copy link
Contributor Author

Sakai-san commented Apr 22, 2021

Again, no strong opinion either way. But I think we should come to a conclusion and an end with this topic eventually.

@neonnoon Ok. Cool. Thanks for your feedback. Alles i.O.

@neonnoon
Copy link
Contributor

✌️

miejs pushed a commit to miejs/pyrene that referenced this pull request Feb 4, 2022
…rene_IconFont_Fix to main

* commit '6497d514395790165be91ba666697da7797b2dc7':
  Fix missing 'thumbsUp' icon.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants