vercel
parse() should throw instead of returning undefined
What's the reasoning behind parse() returning undefined when given invalid input (e.g. '1 secnd')? In my opinion, an error will be much easier to detect and feels syntactically more appropriate – passing an invalid string is an erroneous usage of the function, after all.
I'd totally be willing to draft a PR if there's agreement an error should be thrown.
It's unclear to me where this occurs. https://github.com/vercel/ms/blob/40fdbd5f03bde1ca09aff2884424f92b41061591/src/index.ts#L94 appears to be the only possible location, is this still an issue?
The handling of failure cases still seems sub-optimal to me. This is re: https://github.com/vercel/ms/issues/184#issuecomment-1213069870.
TL;DR: ms() throws for inputs that are neither of type string nor number; additionally, it throws for empty strings, and for non-finite numbers. For strings that aren't empty but that cannot be parsed, ms() will return NaN (as mentioned above). This complicates error handling unnecessarily.
Given that a major-version release is coming up anyway, I'd recommend to remedy this situation by being consistent: either always throwing, or always returning a specific error value. Additionally, there needs to be documentation in the README on how errors will be indicated to the caller. The TypeScript-specific examples there are unsafe since they don't check for NaN.
Side note: NaN is not a good choice for an error value IMHO because it is also of type number. If the error value were undefined, for example, TypeScript (and IDEs) would remind programmers that they should handle this case. This will prevent many non-obvious bugs and security risks especially when ms() is used for parsing user input.
Perhaps someone at Vercel could comment what they want to do? I'd be happy to contribute any required code/docs changes.
It's especially confusing that it does throw when passed an empty string:
> ms('')
[redacted]/node_modules/ms/index.js:34
throw new Error(
^
Uncaught Error: val is not a non-empty string or a valid number. val=""
And then, despite the error message above, it's not thrown when val is not a valid number:
> ms('wat')
undefined
Could we have a fix to make these cases consistent?
