libphonenumber-js: incorrect TypeScript definitions for TelephoneNumber, Extension, PhoneCode

I would like to suggest potentially reopening this issue. Idiomatic Typescript favors just using the “string” type instead of extending “String”. Most people using Typescript will expect the simple string type. Extending String should be left to the user and not required for everyone having to use the library. In most cases, the rest of the program will expect to consume the value as a regular string type. You are making a big assumption that people need to know the difference between CountryCode and TelephoneNumber at the type level. If they do, it should be on them to enforce that.

The assignment needs a cast: const phoneCodeAsString: string = <string>phoneCode;

The problem with Typescript’s export type XXX = string syntax is that the Intellisense of every IDE I’ve ever seen will show a var/property of type XXX as being of type string, not XXX. That largely defeats the purpose of typing it at all. (I believe the syntax doesn’t actually make a new type; it’s just a shortcut to save typing. Hence it doesn’t throw type mismatch errors.)

The reason that requiring a cast is (IMHO) a good thing is such: If you have an Address1 type descended from string and a CustomerName type descended from string, and you assign a value of one to a property/var of the other, you want to get the compiler error that requires a cast, because they’re different types despite both being represented as string.

This problem is particularly acute with string manipulation libraries like this one. Strings in, strings out, did this string already go through the library or not, does the phone# format that that string is in match the format that this string function accepts… everything’s a damn string. You’re effectively working with untyped data since string is about as helpful in this context as any.

Making subtypes that appear in intellisense helps a lot with simple usability, and the occasional required cast, albeit annoying, can save a you a bug hunt or two next week.

But yeah I really wish Typescript would realize that var supertypeVar = subtypeValue is a valid no-cast-required assignment by OO rules.

If we don’t ever assign a NationalNumber variable to an E164Number one

No one should be assigning a “national (significant) number” to an E.164 because those’re two different concepts. Ok, thx.

If we don’t ever assign a NationalNumber variable to an E164Number one, then these opaque types are actually the better way, so no objections from me.

@catamphetamine

Something like parsePhoneNumber() return type being incorrect?

Yes, my concern is with the below type definitions extending String (notice .number in my code which is E164Number):

export interface E164Number extends String { }
export interface NationalNumber extends String { }
export interface Extension extends String { }
export interface CarrierCode extends String { }
export interface CountryCallingCode extends String { }

The Map example was meant to illustrate how extending String is different from having some types strict (the Map code itself doesn’t really represent my actual usage).

The reason the current type definition extends String is because it’s not possible to extend string primitive. However, String wrapper object type and string primitive type are, strictly speaking, different types and do behave differently in some cases.

const s: String = new String('a') // valid
const s: string = new String('a') // invalid, Type 'String' is not assignable to type 'string'.

const stringObj = new String('a')
stringObj instanceof Object // true
stringObj instanceof String // ture
'a' instanceof Object // false
'a' instanceof String // false

So, if some function really expects an object and you give it a E164Number, TypeScript doesn’t complain but behaves differently from what TypeScript thinks it will. This behavior is what I think is not a good trade-off because it can be hard for users to understand why it happens.

So, my proposal is: Make the above types string and maybe add some JSDoc to help IDE users:

export type E164Number = string // keep these types for compatibility and for internal code clarity

export class PhoneNumber {
  /** an E164 number **/ // add some JSDoc comments to help IDE users
  number: E164Number;
}

Users can keep using string alias names, but they will b used for cosmetic reasons and doesn’t affect TypeScript

const phoneNumber: E164Number // this will be the same as const number: string

Published libphonenumber-js@1.9.48.

To clarify, any type defined as export interface XXX extends String { } is also accepted in functions that expect an object, without raising any warnings or errors?

Yes. String wrapper objects inherit from Object and are objects, so they are accepted in functions and variables that expect an object.

interface Str extends String {}
function fn(value: object) {
}

const s:Str = 'a' as Str
fn(s) // no error
fn('a') // Type 'string' is not assignable to type 'object'.ts(2322)

const s1: object = s // no error
const s2: object = 'a' // Type 'string' is not assignable to type 'object'.ts(2322)

I can see that comment having 6 upvotes from which I conclude that other people agree with that approach. And @atsu85 's original comment doesn’t seem to have any upvotes from which I could conclude that no other people actually are strongly supportive of his proposition. So, it seems logical for me to leave the current typings as is.

Understood. Thanks anyway for consideration and maintaining a great library!

@javascripter Thanks for the detailed explanation.

Users can keep using string alias names, but they will be used for cosmetic reasons and doesn’t affect TypeScript

I’d probably agree that “cosmetic” typing would be a better way than strict typing if I was a TypeScript user myself. But I’m not. So it’s not for me to decide.

To clarify, any type defined as export interface XXX extends String { } is also accepted in functions that expect an object, without raising any warnings or errors? That could be considered a bug, I guess.

And you’re proposing we change all extends String { } things to = string.

I can see that comprehensive comment by @RonNewcomb above where he describes why did he choose String {} over string: https://github.com/catamphetamine/libphonenumber-js/issues/170#issuecomment-363156068

I don’t have the qualification to make any decision there. I can see that comment having 6 upvotes from which I conclude that other people agree with that approach. And @atsu85 's original comment doesn’t seem to have any upvotes from which I could conclude that no other people actually are strongly supportive of his proposition. So, it seems logical for me to leave the current typings as is.

@catamphetamine I’m sorry. I should have described my opinion in a more relaxed and neutral way like you suggested. I edited my comment above not to offend others.

Since I only use a few functions from this library, I simply decided to re-export functions with a more strict typing.

import parsePhoneNumber_, { CountryCode } from 'libphonenumber-js'

export const parsePhoneNumber = parsePhoneNumber_ as unknown as (
  text: string,
  defaultCountry?:
    | CountryCode
    | {
        defaultCountry?: CountryCode
        defaultCallingCode?: string
        extract?: boolean
      }
) => string | undefined

thanks for merged PR and maintaining this library 😃

good job