fingerpich / jalali-moment

Display, parse, manipulate, validate and convert jalali (Persian, Khorshidi, Shamsi) or Gregorian (Miladi) dates and times.
https://fingerpich.github.io/jalali-moment/
MIT License
433 stars 68 forks source link

Relative Time Wrong Calculation #49

Closed ammoradi closed 5 years ago

ammoradi commented 5 years ago

Detailed description

I faced this error when i use .fromNow() method; start date: "2018-10-25T13:20:00+00:00" should return: 1 ساعت پیش (for my test time) result: در 621 سال it seems the library calculated subtract of 2018 and 1397!

Steps to reproduce

let fromNow = moment("2018-10-25T13:20:00+00:00").locale('fa').fromNow()
// or without locale()
console.log(fromNow)
fingerpich commented 5 years ago

Dear amir mohammad

I think you have set the locale globally like moment.locale('fa') try the following code

let fromNow = moment.from("2018-10-25T13:20:00+00:00", 'en').locale('fa').fromNow()
console.log(fromNow)

Demo

pouriaMaleki commented 5 years ago

Can we discuss more on this @fingerpich ?

I think as ISO Date is a standard date format, it might be better to not treat it as locale date, but a standard date in 'en' locale.

I say this because it's very common to use this standard to get date/time from db/server and show it in locale format, and using it like below, might add a lot of moment.from(time, 'en').locale('fa') into code instead of simply moment.from(time) when time is ISO standard and locale is already globally sat.

let fromNow = moment.from("2019-01-17T08:19:19.975Z", 'en').locale(currentActiveLocale).fromNow()
console.log(fromNow)

Yet I found nothing about this in moment docs, and didn't check other calendar implementations of moment.js.

I have created a branch with a sample code to fix this on my fork, but it's better to know your opinion about it, then I create pull request for that.

fingerpich commented 5 years ago

Thanks a lot but as long as you didn't change the global locale you can use it as easy as the following code

let fromNow = moment("2019-01-17T08:19:19.975Z").locale('fa').fromNow();
console.log(fromNow)

you know it's a rare thing that you need to change global locale to fa for example when you have lots of code such as moment(specificDate) and specificDate are used to be a gregorian date but now it's going to be jalali date like the situation we have in converting a gregorian calendar to shamsi calendar, we don't want to change all those codes so we can easily change the global locale and it works. So when you need to change the global locale to fa you have to use method from for parsing the gregorian dates.

pouriaMaleki commented 5 years ago

I generally agree with you, in jalali-moment, 'fa' locale is using total different calendar, so parsing dates also happen in same 'fa' calendar (and locale).

Can we make this as an option? Like user can configure jalali-moment to use gregorian calendar for parser even when locale is set to 'fa' and formatter is using Persian calendar?

moment.locale('fa', { useGregorianParser: true });
// use it everywhere like:
moment("2019-01-17T08:19:19.975Z").format("YYYY MM DD"); // "1397 10 27"

This will save our UI code base a lot of repetitive code, as right now I have two options:

moment.locale(navigator.language); // lets assume it's 'fa'
// use it everywhere like
moment(new Date("2019-01-17T08:19:19.975Z")).format("YYYY MM DD"); // "1397 10 27"

or

moment.locale('en');
// use it everywhere like
moment("2019-01-17T08:19:19.975Z").locale(navigator.language).format("YYYY MM DD"); // when navigator.language is 'fa' -> "1397 10 27"

Or even it gets better if moment accepts other calendar implementations and gets configured by locale options like:

import moment from 'moment';
moment.locale('fa', { useNativeCalendarFormatter: true, useNativeCalendarParser: false });

and this will be huge benefit when we want to support users from Arabic countries with pretty native Islamic calendar.

// no need to import other moment plugin as npm packages 
moment.locale('ar', { useNativeCalendarFormatter: true, useNativeCalendarParser: false });

Because right now I have to create a wrapper around moment, jalali-moment and something like moment-hijri to support all these different calendars for MiddleEast users. ``

fingerpich commented 5 years ago

Yes it is a good idea and helpful if we consider that without this option it should do the same thing as before. if you can create a pull request and i will help you. About moment I think its more than this as you can see in this issue