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

Timezone / Summer time support #8

Closed
chdanielmueller opened this issue Apr 12, 2019 · 10 comments · Fixed by #9 · May be fixed by #10
Closed

Timezone / Summer time support #8

chdanielmueller opened this issue Apr 12, 2019 · 10 comments · Fixed by #9 · May be fixed by #10

Comments

@chdanielmueller
Copy link
Contributor

Hi @xpepermint ,

As far as I can observe the library always works with UTC timestamps in the sleepUntil and interval field.
Is it possible to configure the library at the current state to react to summer time changes?

If not here is my idea:
I can see that the library uses moment.
In my code I use moment with the timezone extension. (moment-timezone)
I suppose with changing the imports from import moment from 'moment'; to import moment from 'moment-timezone'; and adding a default timezone moment.tz.setDefault('Europe/Zurich'); the behaviour could be altered.
Of course this would need a new option in the constructor. e.g. new MongoCron({..., timezon: 'Europe/Zurich'})

I didn't test it and have not looked deep into the code.
What do you think?

Thanks,
Daniel

@xpepermint
Copy link
Owner

We can add configuration options. I'll look into this asap. You can post a PR proposal here to speed up things.

@chdanielmueller
Copy link
Contributor Author

@xpepermint Sorry for bothering you again.
I propose a 7 line change which would fix this issue for me at #9.

I did use a function of later which allows to use system local time zone instead of UTC.
http://bunkat.github.io/later/ -> Step 3: Configure timezone

It would be great if you could take a look at the PR and publish a new version if you are fine with it.

@xpepermint
Copy link
Owner

I promise I'll do my best to jump on this asap. I'm sorry you have to wait ...

@xpepermint
Copy link
Owner

#9

@xpepermint
Copy link
Owner

Unfortunately, I realized that your PR won't work. It would work but there's a hidden issue with it. The problem is that the LateJS changes package behavior globally. I think I'll have to replace LaterJS with something like https://github.com/harrisiirak/cron-parser which would also add full support for timezones. This will take some time but I'm on it ...

@xpepermint xpepermint reopened this Apr 24, 2019
@xpepermint
Copy link
Owner

I refactored the code and replaced LaterJS with CronParser. Timezone can now be added easily but I wonder, do we really need that since this year we've changed our clocks for the last time (at least in Europe).

@xpepermint
Copy link
Owner

xpepermint commented Apr 24, 2019

Also, don't forget that you can have dates with timezone in the database and it will work. Any ISO 8601 is a valid date formate. Good practice, however, is to always use UTC when storing to DB.

@chdanielmueller
Copy link
Contributor Author

@xpepermint Thank you for looking at this.

I would be totally fine with storing the date in UTC to the database.
My issue is the interpretation of the intervals in the specific timezone.

We changed our clocks for the last time? I must somehow missed this. As far as I know it has just been decided to do something about it with the current target for last change in 2021.

@chdanielmueller
Copy link
Contributor Author

@xpepermint I did add a new proposal (#10) to implement this feature.
Please take a look if you have time.

@xpepermint
Copy link
Owner

OK ... moving to the PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants