Document and test `TOO_SHORT` parsePhoneNumber error

[nobitagit]

Hi,

first of all many thanks for writing and sharing this library.

I noticed that parsePhoneNumber can (correctly) return a fourth error, TOO_SHORT, other than the three currently documented in the README under possible errors.

Try this for example:

const { parsePhoneNumber } = require('libphonenumber-js');

// Country code is valid, but number is < 2 digits
const ret = parsePhoneNumber('+443');
// > Error: TOO_SHORT

Hence, I added this error to the docs and a further test case to to the relevant test file. Hope that's alright.

Cheers.

[catamphetamine]

@nobitagit Oh, thanks. Good pull request. It's funny that when I wrote that list of possible errors I tried to reproduce TOO_SHORT and didn't find a test case throwing such error so I decided: "Seems that TOO_SHORT can never be thrown, but I won't remove it from the code just yet". Seems that the issue was that I tested Russian and USA numbers both of which have one-digit country calling code, and you managed to find the test case with two-digit country calling code when it does throw that error.

[catamphetamine]

@nobitagit By the way, what's your opinion on the current parsePhoneNumber() function usability? I find it weird that it requires a try/catch wrapper. I was thinking to introduce a replacement function, something like parseNumberFromString() which would simply call parsePhoneNumber() internally wrapped in a try/catch wrapper, and if there's an error then it would return undefined.

let number
try {
  number = parsePhoneNumber('123', 'GB')
} catch (error) {
  return
}
doSomethingWithNumber(number)

vs

const number = parseNumberFromString('123', 'GB')
if (!number) {
  return
}
doSomethingWithNumber(number)

Seems more convenient. What do you think?

Conceptually, it shouldn't be viewed as an error if the phone number is incomplete I think. I.e. why would it be an error? In a sense of "an emergency situation". "Something not going as planned".

[nobitagit]

It's curious you ask, as I was thinking the same about the try/catch.

I 100% agree with your logic that an incomplete number should not throw. I personally am not a big fan of throwing errors unless the input is completely in the wrong format or something really unexpected happened while processing.

I would say, a string with an incomplete/incorrect number is definitely something this library supports and understands, so it can be dealt with without throwing errors. Basically, as long as the input is of type string (and number?) we could consider it ok. Just, if it happens to be a non legit number we return the reason without throwing.

phoneNumberErrors('+4432123232'); // ok
phoneNumberErrors('blabla'); // still ok, we return the error code without throwing
phoneNumberErrors('+0000000'); // still ok, we return the error code without throwing

phoneNumberErrors({ some: 'stuff' }); // can't deal with objects, throw an error?
phoneNumberErrors([1,2,3,4,5]); // throw, same as above
phoneNumberErrors(Symbol('stuff')); // throw, same as above

Now, regarding your idea of doing this:

const number = parseNumberFromString('123', 'GB')
if (!number) {
  return
}
doSomethingWithNumber(number)

It could work but then, as a consumer of the lib, I would lose the info about the reason why the number is not right. So, I would be unable to surface a meaningful error to the user. Basically rather displaying messages such as "the input number is too long", or "the country code is not valid", I could only return "the number is not correct", which is still ok, but a less pleasant user experience.

In that sense, what about always returning an object with all the metadata we can gather, error code included. For example:

  parsePhoneNumber('Phone: 8 (800) 555 35 35.', 'RU')
 /*
{
   country: 'RU',
   number: '+78005553535',
   isValid: true,
   type: 'TOLL_FREE',
   error: null,
  .... // any other info we can return to the consumer
}
*/

  parsePhoneNumber('+44565656566565656565656565');
 /*
{
   country: 'UK',
   number: '+44565656566565656565656565',
   isValid: false,
   type: null,
   error: 'TOO_LONG',
  .... // any other info we can return to the consumer
}
 */

A lot of validation libraries take this approach, so the consumer is free to do whatever he wants, as he has access to a rich set of metadata regarding the input.

const result = parsePhoneNumber('+44565656566565656565656565');

// maybe I am just interested in whether the number is valid or not?
return result.isValid;

// maybe i still want to do something with the country code?
const { error, country } = result;
return { error, country };

// or, if I really want to throw an error, I can still do so
if (result.error != null) {
   throw new Error(result.error);
}

As an extension to this logic, one could argue that a single string could be wrong for more than one reason. So, why not returning an array of errors?

{
   country: 'UK',
   number: '+00565656566565656565656565',
   isValid: false,
   type: null,
   errors: [{ code: 'TOO_LONG' }, { code: 'INVALID_COUNTRY' }]
  .... // any other info we can return to the consumer
}

But that's perhaps looking too far ahead. Also, I want to stress that even in the current implementation I could still use this lib very easily, so we're talking about making the ergonomics better, not fixing something that's broken.

Sorry for the long reply, hope some of this makes sense to you. Also, it would be beneficial to hear other people opinions about this perhaps.

[catamphetamine]

@nobitagit Thx for the detailed opinion. I've settled on adding parsePhoneNumberFromString() function for now (which doesn't throw but instead returns undefined). Released the new version. Developers wanting to know the exact reason can use the old parsePhoneNumber() function.