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

Problem with Business Day calculation #109

Open
tscislo opened this issue Apr 13, 2023 · 8 comments
Open

Problem with Business Day calculation #109

tscislo opened this issue Apr 13, 2023 · 8 comments

Comments

@tscislo
Copy link

tscislo commented Apr 13, 2023

import moment from "moment-business-days";

moment.updateLocale('us', {
    workingWeekdays: [1, 2, 3, 4, 5]
});

console.log(moment('2021-04-10', 'YYYY-MM-DD').isBusinessDay());
console.log(moment('2021-04-11', 'YYYY-MM-DD').isBusinessDay());

console.log(moment("2021-04-05").businessDiff(moment("2021-04-09"))); // Friday
// LOGS 4
console.log(moment("2021-04-05").businessDiff(moment("2021-04-10"))); // Saturday
// LOGS 5
console.log(moment("2021-04-05").businessDiff(moment("2021-04-11"))); // Sunday
// LOGS 5

In my opinion in all cases it should log 4, as Saturday and Sunday are not working days.
Do I miss something here?

@tscislo
Copy link
Author

tscislo commented Apr 13, 2023

Apparently this is a bug in 1.2.0 version,
In version 1.3.0 it does not happen
console.log(moment("2021-04-05").businessDiff(moment("2021-04-09"))); // Friday
// LOGS 4
console.log(moment("2021-04-05").businessDiff(moment("2021-04-10"))); // Saturday
// LOGS 4
console.log(moment("2021-04-05").businessDiff(moment("2021-04-11"))); // Sunday
// LOGS 4

Please publish version 1.3.0 to npm. Thx!

@mcdado
Copy link
Collaborator

mcdado commented Apr 14, 2023

Yes, @eduolalo should release a new version on npm, since he's the only one with access rights. In the meantime, you can use github links like this:

"dependencies" : {
  "moment-business-days" : "https://github.com/eduolalo/moment-business-days.git#commit-ish",
}

@tscislo
Copy link
Author

tscislo commented Apr 14, 2023

Here is the one I published https://www.npmjs.com/package/@mobile-next/moment-business-days if anyone needed

@davidworkman9
Copy link

This still seems to happen for me in my testing on the newer version. Just seems to happen less often though. Example code:

(async () => {
    for (let i = 0; i < 500; ++i) {
        const diff = moment(moment().businessAdd(6)).diff(moment(), 'days');
        if (diff !== 6) {
            console.log('BROKEN!!!');
        }
        await new Promise(resolve => setTimeout(resolve, 25));
    }
    console.log('done');
})();

Seems to somehow be dependent on the ms of the current time oddly enough because if I change moment().businessAdd(6) to moment().startOf('day').businessAdd(6) it starts passing.

@mcdado
Copy link
Collaborator

mcdado commented Dec 7, 2023

I'm not sure actually on how moment.diff works, if let's say 23:59 yesterday is diffed against 00:00 today, when rounded to days, returns 1 or 0. If you need, you can either use startOf day for both like you mentioned or set a fixed time like noon to both moment objects to work around the issue.

@davidworkman9
Copy link

@mcdado Ah, that makes perfect sense. Thank you.

@dpobel
Copy link

dpobel commented Jan 9, 2024

got exactly the same problem. It would be nice to publish the 1.3.0. @eduolalo @mcdado any chance to have a 1.3.0 release soon?

@mcdado
Copy link
Collaborator

mcdado commented Jan 10, 2024

I guess if @eduolalo won't show up I'll publish it under a different npm name.

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

No branches or pull requests

4 participants