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

Fix features - dependencies - timezone - variable scope #248

Merged
merged 11 commits into from Mar 10, 2021

Conversation

flopal
Copy link
Contributor

@flopal flopal commented Sep 17, 2020

Hi,

I use node v14.5.0 and npm 6.14.8, if I can help to debug.

This PR updates a lot of things.

  1. dependencies. I used npm-check library to upgrade all dependencies and to fix some audit issues. see commit
    6f2617b.

  2. timezone. When I was running the npm test command, some tests failed about the timezone. I fixed it by replacing tz-offset to moment-timezone and updating some files (see the list item in commit 72a454a and change in commit 4f26623).

  3. variable scope. I replaced all var to let or const (depend on the variable update). See commit 27e1140.

  4. readme. As I said on 1), I changed the timezone library, so I updated the link / Text in the readme. See commits b487616 and 13f9022 (bad copy-paste in the first commit)

I ran again the npm test to check if everything is alright: all tests are passed, but the first test can be failed (or be passed) during the first iteration (depend on delay to reach the result [deamon.js]).

@coveralls
Copy link

coveralls commented Sep 17, 2020

Pull Request Test Coverage Report for Build 360

  • 64 of 64 (100.0%) changed or added relevant lines in 9 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage remained the same at 100.0%

Totals Coverage Status
Change from base Build 357: 0.0%
Covered Lines: 274
Relevant Lines: 274

💛 - Coveralls

@abdurrahmanekr
Copy link

Hey hi 👋, this repo is insufficient on timezone. If this PR solves this problem, is there any reason not to merge it?

@flopal
Copy link
Contributor Author

flopal commented Feb 8, 2021

Hi ! 👋

Thank you for your interest to my PR (and this project of course) 🙏

I guess this project is not maintained or it takes time to find another maintainer. Check this issue #255

Moreover, my PR could be better - replace moment-timezone to another package (refer to this status on the momentjs website).

  • I worked with Luxon and it's pretty like momentjs - it's a tank !
  • Tz-data could be a better alternative. 💪

I'm waiting for news about this project to know if this project continues or not. I will update my PR if it continues, do nothing (leaves my PR like that) otherwise.

@merencia merencia merged commit 427483c into node-cron:master Mar 10, 2021
@merencia
Copy link
Member

Hi there!

I'm really sorry for the delay.

@flopal thanks for you PR.

@flopal
Copy link
Contributor Author

flopal commented Mar 11, 2021

Don't worry about that @merencia, I understand!

I'm glad to see you back to add a new version on this repo.

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

Successfully merging this pull request may close these issues.

None yet

4 participants