moment.min / moment.max doesn't check the first argument to be valid (isValid())

[ofirmalka]

Describe the bug
moment.min / moment.max doesn't check the first argument to be a valid (isValid())

To Reproduce
Steps to reproduce the behavior:
moment.max('1', moment('2013-01-01')) // return moment('2013-01-01')
moment.max('a', moment('2013-01-01')) // return 'a'

Expected behavior
throw error

Screenshots

Desktop (please complete the following information):

• OS: MacOS
• Browser chrome
• Version 114.0.5735.133

Moment-specific environment

• The time zone setting of the machine the code is running on
• The time and date at which the code was run
• Other libraries in use (TypeScript, Immutable.js, etc)

Please run the following code in your environment and include the output:

console.log((new Date()).toString())
console.log((new Date()).toLocaleString())
console.log((new Date()).getTimezoneOffset())
console.log(navigator.userAgent)
console.log(moment.version)

Sun Jul 09 2023 11:24:42 GMT+0300 (Israel Daylight Time)
7/9/2023, 11:24:42 AM
-180
Mozilla/5.0 (Macintosh; Intel Mac OS X 10_15_7) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/114.0.0.0 Safari/537.36
2.29.4

[ichernev]

The reason is likely NaN comparison. Ideally we should exclude NaN (invalid moments) from the list. Throwing errors is not an option. You can try to submit a PR.

[ofirmalka]

currently if any of the arguments (except the first one) is not a moment object an error will be thrown

> moment.min(1, 2)
global.js:3145 Uncaught TypeError: moments[i].isValid is not a function
    at pickBy (global.js:3145:29)
    at Function.min (global.js:3156:16)
    at <anonymous>:1:8

so why it should behave differently for the first argument?

[ichernev]

function pickBy(fn, moments) {
    var res, i;
    if (moments.length === 1 && isArray(moments[0])) {
        moments = moments[0];
    }
    if (!moments.length) {
        return createLocal();
    }
    res = moments[0];
    for (i = 1; i < moments.length; ++i) {
        if (!moments[i].isValid() || moments[i][fn](res)) {
            res = moments[i];
        }
    }
    return res;
}

because it has to work in some way, so it picks the first and compares with the rest in order.

[ofirmalka]

Yes, I saw, but all other will cause TypeError if not a moment object but the first one won't, so why not just throw an error if the first is not a moment object?
or you think it's better to filter all non moment objects from the array before?

[ofirmalka]

BTW, if (!moments[i].isValid() || moments[i][fn](res)) why res updated when momeent[i] is not valid? 🤔

[ichernev]

BTW, if (!moments[i].isValid() || moments[i]fn) why res updated when momeent[i] is not valid? 🤔

Eh, how many errors can you spot in a random 10 line passage from moment :)