Dynamic calendar system

[Fardinak]

This is a good library, however we need our application to be a little more dynamic i.e. we need to let the user switch between calendar systems. With the current version of moment-jalaali it is only possible through a lot of conditional development i.e. one needs to check for the flag and call the appropriate method (e.g. year or jYear) with the appropriate arguments (mostly formatting).

Following issue #30 I have a proposal. I would suggest using a flag as well as the custom methods, so the following conditions would be true:

var dt = new moment(Date.now(), 'll', 'en', moment.SYSTEM.JALALI),
    currentJalaliYear = 1394,
    currentGregorianYear = 2015;

dt.year() == currentJalaliYear;
dt.format('YYYY') == currentJalaliYear;

dt.switchSystem(moment.SYSTEM.GREGORIAN); // Changes the calendar system to Gregorian
dt.year() == currentGregorianYear;
dt.format('YYYY') == currentGregorianYear;

dt.jYear() == currentJalaliYear;
dt.format('jYYYY') == currentJalaliYear;

dt.gYear() == currentGregorianYear;
dt.format('gYYYY') == currentGregorianYear;

Of course the formatting is just a suggestion which I suspect may even conflict with the week year tokens.

[behrang]

I want this project to be as identical to moment itself as possible. Doing what you proposed, makes maintaining it harder and I don't want to do that.

I suggest you create your own little utility functions which do the condition checking you said and call either year or jYear, or use YYYY format or jYYYY.

[Fardinak]

IMHO, using a flag to manage the calendar system is more integrated. The extra functions can be swapped with new parameters or eliminated completely. My point is that, the calendar system should not affect formatting and method calls.

That being said, it's your decision and I completely respect it. :)

[behrang]

I'm convinced. Let me think a little more about it.

[Fardinak]

Great :+1:

[alitaheri]

I don't think the api needs changing. As it is, it's very readable and very easy to reason about how you want your dates formatted. In my opinion you, if you want to support both calendars in the same application you should follow the Provider Pattern. Take the code below for example:

// dateFormatProvider.js
export default (persian) => ({
  dateOnly: persian ? 'jYYYY/jM/jD' : 'YYYY/M/D',
  dateTime: persian ? 'jYYYY/jM/jD HH:mm' : 'YYYY/M/D HH:mm',
  // ...
});

So you can use it like:

const dateFormatProvider = require('./path/to/dateFormatProvider');

let persian = true;
let date = moment('1391/12/30', dateFormatProvider(persian).dateOnly);
//...

This pattern also wraps your formats inside a single file so you will have consistent formatting throughout your application.

And about the functions... yeah it would have been nice to have consistent api with the moment-js itself :sweat_smile:. So a flag on the object would do. But for the formatting I think this api should stay as it is.

[behrang]

You are right about formatting, but the problem is not formatting. Date pickers that use moment, are calling year, month, ... methods. As currently, moment-jalaali adds some other methods like jYear, jMonth, ... those date pickers need substantial rewrite to call these methods. This makes converting them a lot harder.

I have a plan for adding a flag and a global default to moment-jalaali and override year, month, ... methods, so that depending on the flag, it will switch to Gregorian or Jalaali, and I think this will make integration a lot easier.

[alitaheri]

Yeah it does. but have that flag defaulting to Georgian and keep the j... functions (but deprecate them). Don't introduce unnecessary breaking changes.

I love your library it awesome :D

P.S. if you don't have time for that I can open a PR for you. I love your library and I would like to give back. Your call ^.^

[behrang]

Yes, that would be great and I appreciate your help.

Here is what I thought:

moment object will have a static property (like calendarSystem or system), defaulting to Gregorian. If it is not changed to Jalaali, everything works as normal. If it is set to Jalaali new instances will be created in that system.

When creating new instances, there can be an additional parameter to the constructor (the last parameter), which sets the system for that instance. If it is not provided, it defaults to previous static property.

When an instance is created, it's system can be changed by setting a property on the instance.

When methods of the instance are called, depending on the system, they do the appropriate job. If system is Gregorian, they just call their parent methods. If system is Jalaali, they do the conversion.

Thanks

[alitaheri]

moment object will have a static property (like calendarSystem or system), defaulting to Gregorian. If it is not changed to Jalaali, everything works as normal. If it is set to Jalaali new instances will be created in that system.

I would suggest against this. Because changing that on a global object has global side effect which is always bad. And very hard to trace.

When creating new instances, there can be an additional parameter to the constructor (the last parameter), which sets the system for that instance. If it is not provided, it defaults to previous static property.

If we do it on the constructor every time someone wants an instance of moment they have to invoke or bind calls to the constructor like moment('1360/5/26', 'jYYYY/jM/jD', null, false, 'jalali') or something like that. Since all moment instances are supposedly immutable a context switching might not be a bad idea! Something like:

m = moment('1360/5/26', 'jYYYY/jM/jD');

m.year(); // 1981
m.month(); // 8
m.date(); // 17

// switch context
m.calander = 'jalali';
m.year(); // 1360
m.month(); // 4
m.date(); // 26
m.dayOfYear(); // 150
m.week(); // 22
m.weekYear(); // 1360

// Switch back
m.calander = 'georgian';

m.year(); // 1981
m.month(); // 8
m.date(); // 17

This way, you can easily incorporate this side-effect into external libraries too:

m = moment('1360/5/26', 'jYYYY/jM/jD');
if(persian) m.calander = 'jalali';
require('./some/large/library').prettyFormatDateAndShow(m);
m.calander = 'georgian';
require('./some/database/connection').storeDate(m); // must be Georgian to store efficiently.

Easy, effortless and very easy to trace in case of bugs or errors (you just have to trace the instance, which is easier than tracing static property mutation) :D

Sorry I'm an API freak :D :D convention matters!

If this is ok with you I'll make the required changes (with tests) and open a PR in a day or two.

[behrang]

Yes and No.

The problem is, with this solution, we can't use other date pickers easily. They would still need a rewrite or somehow overwriting their usage of moment.

For example, imagine that there is a date picker field, and user can enter a text into the text input field and then click on the picker icon to show the picker. Since that picker might have implemented this functionality using normal date formatting string like this:

m = moment(userInput, 'YYYY/M/D')
// use m to update picker month view

then we can't use it easily with Jalaali calendar system.

I think we need a breaking change and change major version number. Then remove jYYYY, jM, ... formatting strings. And then somehow, depending on something else, decide weather parsing and formatting should be done in Gregorian or Jalaali.

When I created the project, I thought that having this ability to use a formatting string like YYYY/M/D [is] jYYYY/jM/jD is really interesting. But now, I think there is no actual real world use for this. Instead, having the ability to switch between calendar systems (like what you proposed) is a lot more useful. So, we can discard j prefixed format and parse strings and use normal format and parse strings.

What do you think?

[alitaheri]

Hmmm... I understand your point. Although we still don't need to remove those formats just yet. Deprecate them (show warning if they are used) and remove them with the next major release at least 1-2 months from after you deprecate them. Switch formatting context with that flag but still keep those formats (deprecate and document that the next major release removes them). Increment the minor version number and after a month or two remove them in the major release.

Sound good?

Do I get to work? :D

P.S. I don't know the api moment gives... does it allow overriding formatters? I mean... can we actually even change the behavior of YYYY or MM formatters?

[behrang]

If I was going to implement it, I would remove them and increase major version number. Users of the old version won't notice since package managers will not automatically upgrade to the new major version, and those who choose to upgrade, will be required to change their usage. I think this is safer, since they will notice it explicitly. I think they might miss or ignore deprecation warnings, and suddenly, by an automatic minor version update, their apps will stop working.

But the decision is yours. Your approach also works and users are familiar with deprecation concept.

About your last question, it doesn't allow overriding formatters. In current implementation, some methods are overridden. For example, format method first replaces j prefixed formatters with Jalaali values, and then calls super method and let it replaces Gregorian formatters.

Anyway, thanks for your time.

[alitaheri]

I think you misunderstood. What I'm saying it that you should increment one minor version deprecating them and adding the new features. Then with the next major release remove them. This way, their app will not fail, only show warning saying Next major release will remove this, use that instead. This prevents chaos. They are warned one or two months before the breaking changes are introduced. Instead of saying "If you want the shiny new version you have to review a million lines of code like RIGHT NOW" we say "if you want shiny new futures from now on you have one or two months to adept your code, and if you don't want those features then just don't do anything" :D

So I will mark the current formatting as deprecated, add the new functionality and then when you wish to release the next major version, you can remove the deprecated codes.

Okay then, I will try to work around that problem. Thanks a lot :+1:

I'll get to work as soon as I can ^^

[behrang]

Great. Thanks.

On Thu, Oct 8, 2015 at 8:01 PM, Ali Taheri Moghaddar notifications@github.com wrote:

I think you misunderstood. What I'm saying it that you should increment one minor version deprecating them and adding the new features. Then with the next major release remove them. This way, their app will not fail, only show warning saying Next major release will remove this, use that instead. This prevents chaos. They are warned one or two months before the breaking changes are introduced. Instead of saying "If you want the shiny new version you have to review a million lines of code like RIGHT NOW" we say "if you want shiny new futures from now on you have one or two months to adept your code, and if you don't want those features then just don't do anything" :D So I will mark the current formatting as deprecated, add the new functionality and then when you wish to release the next major version, you can remove the deprecated codes. Okay then, I will try to work around that problem. Thanks a lot :+1:

I'll get to work as soon as I can ^^

Reply to this email directly or view it on GitHub: https://github.com/jalaali/moment-jalaali/issues/31#issuecomment-146606010

[alitaheri]

After spending some days trying to achieve what we discussed I bumped into an ugly truth. Moment.js itself is dependent on the api it provides! We can't touch some of the methods. Even if we do touch them and do some ugly hacks to get around it, we can't guarantee that our code will work with different versions of their api. take for example this:

I traced the add function to this line:

dayOfMonth = Math.min(mom.date(), daysInMonth(mom.year(), value));

I did finish the functions with some hacks! but hacks are bad ideas! If we actually want the api we discussed we have to come up with some entirely different solution -_- I never expected moment.js to use it's own api like that :|

You can pull my fork and take a look. I'm not going to make a PR.

[behrang]

I looked at your fork, and it looks great and your hacks are great too :)

And I agree with you, but I don't have any ideas for a different solution. Your change is working now, but formatters are not changed. What should we do? Do you have any ideas?

[alitaheri]

Thanks ^^ All I can come up with is the constructor constructor. If we want to do this right we have to respect the api. We can have 2 simultaneous approaches. One being a constructor that can create 2 constructors one only constructing Gregorian moments with Gregorian static functions, and the other one only constructing Jalaali moments with Jalaali static functions. This way we can have exactly identical apis across the both realms. everything will be the same, except for the names, formats, inputs and outputs. all function names, properties and formats will remain identical. The other approach is the context switching. We should have functions returning new instances of the other realm. For example:

// To have both constructors simultaneously.
var gMoment = new moment.Gregorian();
var jMoment = new moment.Jalaali();

var gM = gMoment('1981/8/17', 'YYYY/M/D');
var jM = jMoment('1360/5/26', 'YYYY/M/D');

// To be able to switch constructors seamlessly.
// A little setup.
var Moment = moment;
Moment.g = new Moment.Gregorian();
Moment.j = new Moment.Jalaali();

// Make Gregorian the default.
moment = Moment.g;

var gM = moment('1981/8/17', 'YYYY/M/D'); // Gregorian instance

// Later on, switch to Jalaali:
moment = Moment.j;

var jM = moment('1360/5/26', 'YYYY/M/D'); // Jalaali instance

jM.equals(gM) // true

// To switch instances: (Will also respect immutability)
var gm = gMoment.fromJalaali(jm);
var jm = jMoment.fromGregorian(gm);

This will introduce a whole lot of breaking changes. I don't think we can deprecate the old api, we should outright replace it :grin:

[behrang]

Looks good. Please, go for it.

[alitaheri]

Alright, I'll try.

[alitaheri]

Nope... The only solution I came up with is the whole rewrite of the entire library so that it always keeps an original moment object and proxies all the operations with input/output conversion. There is no way to override the original api without introducing catastrophic side effects! So, the solution is to rewrite the library, and follow the Composition/Forwarding Pattern. I don't have the time for that unfortunately :cry:

[behrang]

Ok, nice try. Thanks anyway.

[alitaheri]

Very hard to implement. So it's a no-go