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

Migrated to Day.js from Moment.js. #196

Closed
wants to merge 6 commits into from

Conversation

JeppeLarsen
Copy link

(This should resolve issue #195 )

I have tried to migrate from Moment.js to Day.js, because Moment.js is no longer being supported and because Day.js has a lot smaller bundle size. See this for comparison: Day.js bundlephobia vs Moment.js bundlephobia.

PLEASE NOTE: Not all tests pass - probably because of some difference between Day.js and Moment.js in the way time zones are being handled.
The tests that do not pass are:

  • expression.js from line 1301 to line 1321
  • timezone.js

Perhaps someone can look into these bugs.

@harrisiirak
Copy link
Owner

@JeppeLarsen thanks for PR!

Not all tests pass - probably because of some difference between Day.js and Moment.js in the way time zones are being handled.

Getting timezone stuff working as before is important. I'll also try to have a look why these tests started to fail.

@JeppeLarsen
Copy link
Author

@JeppeLarsen thanks for PR!

Not all tests pass - probably because of some difference between Day.js and Moment.js in the way time zones are being handled.

Getting timezone stuff working as before is important. I'll also try to have a look why these tests started to fail.

Thanks for your reply.

I have been trying to debug it and found out that the function "addHours" in date.js is doing strange things when running the test in expression.js from line 1301 to line 1321. It is actually subtracting an hour in my timezone instead of adding one.
The tests in timezone.js seem to live their own lives, maybe it is the same root cause.

I cannot seem to figure out if the problems are general issues with day.js or something that could (or should) be solved in this library. I am looking forward to your opinion on this.

This solves the strange behaviour of the addHour-function.
All tests in expression.js now pass.
@JeppeLarsen
Copy link
Author

@JeppeLarsen
I have been trying to debug it and found out that the function "addHours" in date.js is doing strange things when running the test in expression.js from line 1301 to line 1321. It is actually subtracting an hour in my timezone instead of adding one.

I solved this. It was a problem with Day.js being immutable.
Although it did not solve the problems with the timezone.js-tests, I think we are one step closer.

@Uzlopak
Copy link

Uzlopak commented Dec 6, 2020

It seems that interval.next does not respect the localOffset change

@harrisiirak
Copy link
Owner

@JeppeLarsen FYI, there is now also an open PR with luxon as replacement to moment-timezone: #202 -- all the tests (including TZ support) are passing, this is a big bonus.

I'm really considering moving on with luxon at this point, although I'm still interested getting the bundle even smaller, and therefore if we're able to get Day.js perfectly working with timezone support, we can to switch to Day.js at some point.

@andytson
Copy link
Contributor

andytson commented Jan 1, 2021

https://day.js.org/docs/en/parse/string

For consistent results parsing anything other than ISO 8601 strings, you should use String + Format.
https://day.js.org/docs/en/parse/string-format

Hope that helps. e.g. in #202 the CronDate constructor is matching multiple format patterns to account for a number of formats that JS Date supports (though inconsistently in browsers aka the deprecation warning in moment says

Deprecation warning: value provided is not in a recognized RFC2822 or ISO format. moment construction falls back to js Date(), which is not reliable across all browsers and versions. Non RFC2822/ISO date formats are discouraged. Please refer to http://momentjs.com/guides/#/warnings/js-date/ for more info.

Main things I have against Day.Js though (although with 1 or less locales it can be smaller) are:

  • it's plugin and locale system is a bad fit in typescript
  • if many or user-customisable locale are used, it may end up larger than luxon

There's no answer that will make everyone happy though, so I'd understand

@andytson
Copy link
Contributor

andytson commented Jan 1, 2021

I've tried to do the work needed for the formats for this branch, but with moment and luxon supporting RFC2822 https://github.com/moment/luxon/blob/master/src/impl/regexParser.js, I think it'd be a stretch to support that format with dayjs.

"testParser": "tap ./test/parser.js",
"testLy": "tap ./test/leap_year.js",
"testIofi": "tap ./test/increment_on_first_iteration.js",
"test31": "tap ./test/31_of_month.js"
Copy link
Contributor

@andytson andytson Jan 1, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you can do npx tap ./test/expression.js rather than list every test as a script

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am sorry for bloating the package.json with this. These lines were not supposed to be pushed.

@JeppeLarsen
Copy link
Author

@JeppeLarsen FYI, there is now also an open PR with luxon as replacement to moment-timezone: #202 -- all the tests (including TZ support) are passing, this is a big bonus.

I'm really considering moving on with luxon at this point, although I'm still interested getting the bundle even smaller, and therefore if we're able to get Day.js perfectly working with timezone support, we can to switch to Day.js at some point.

You should move on with luxon for now. We can switch to Day.js, when/if it becomes mature enough to include the timezone-support.

@JeppeLarsen
Copy link
Author

https://day.js.org/docs/en/parse/string

For consistent results parsing anything other than ISO 8601 strings, you should use String + Format.
https://day.js.org/docs/en/parse/string-format

Hope that helps. e.g. in #202 the CronDate constructor is matching multiple format patterns to account for a number of formats that JS Date supports (though inconsistently in browsers aka the deprecation warning in moment says

Deprecation warning: value provided is not in a recognized RFC2822 or ISO format. moment construction falls back to js Date(), which is not reliable across all browsers and versions. Non RFC2822/ISO date formats are discouraged. Please refer to http://momentjs.com/guides/#/warnings/js-date/ for more info.

Main things I have against Day.Js though (although with 1 or less locales it can be smaller) are:

  • it's plugin and locale system is a bad fit in typescript
  • if many or user-customisable locale are used, it may end up larger than luxon

There's no answer that will make everyone happy though, so I'd understand

Thank you. I will look into the formatting-thing.

@harrisiirak
Copy link
Owner

Closing this for now.

@harrisiirak harrisiirak closed this Oct 8, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants