eduolalo / moment-business-days

This is a momentJS plugin that allows you to use only business days (Monday to Friday)
MIT License
240 stars 67 forks source link

Problem with Business Day calculation #109

Open tscislo opened 1 year ago

tscislo commented 1 year ago
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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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

davidworkman9 commented 9 months ago

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 commented 9 months ago

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 commented 9 months ago

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

dpobel commented 8 months ago

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 commented 8 months ago

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