incorrect TypeScript definitions for TelephoneNumber, Extension, PhoneCode

[atsu85]

EDIT: it turned there was serious downside with this change i proposed bellow

Problem

Currently PhoneCode (and TelephoneNumber and Extension) are defined using interface, like:
export interface PhoneCode extends String { }
meaning, that variables/fields of these types are treated as objects, not string primitives
(even tough typeof phoneCode === "string").

As a result variable of type PhoneCode can't be assigned to variables/fields with string type:

import { getPhoneCode } from 'libphonenumber-js';

const phoneCode = getPhoneCode('EE');
const phoneCodeAsString: string = phoneCode;
// ERROR:^^^^^^^^^^^ TS2322: Type 'PhoneCode' is not assignable to type 'string'.

Expected behaviour

Should be able to asign PhoneCode (and TelephoneNumber and Extension) to ´string´.

Solution

Declare PhoneCode (and TelephoneNumber and Extension) using
export type XXX = string
instead of
export interface PhoneCode extends String { }

---

Just in case, I'm mentioning @RonNewcomb, because he created the initial TypeScript definitions file, and also @catamphetamine who has created Extension type by following example of @RonNewcomb

[catamphetamine]

good job

[atsu85]

thanks for merged PR and maintaining this library :)

[RonNewcomb]

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.

[atsu85]

@RonNewcomb, good point - it is a matter of trade-offs. According to my change it was possible to just document the type, allowing to assign it to string without casting, but the downside is that it won't prevent me from assigning one kind of string to different kind of string either.

I prepared a PR that reverts my previous change.

[atsu85]

@catamphetamine, sorry for the inconvenience - please merge my PR that reverts my previous change related to this issue

[catamphetamine]

Released libphonenumber-js@1.0.12

[catamphetamine]

@atsu85 Thx for trying
@RonNewcomb Thanks for clearing things up

[jineshshah36]

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.

[jineshshah36]

I'm happy to make the changes if we can agree

[javascripter]

As String objects aren't exactly the same as string primitives, treating it as such could introduce subtle unexpected type-unsafe behaviors to user programs potentially in the form of a runtime error which TypeScript could otherwise have detected.

For example, the following compiles because number is typed as subtype of String object but throws in runtime:

const number = parsePhoneNumber(phoneNumber).number
const m = new WeakMap()
m.set(number, '1') // a runtime error because WeakMap#set only accepts objects and not primitives

It's possible users are not aware of this subtle type unsafeness introduced by this type definition because as far as I know it's not a common practice to use String object subtype as nominal types. I think this issue could be reopened for more discussion.

[javascripter]

@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

[catamphetamine]

@javascripter Oh sh*t, you're Japanese. I forgot Japanese are obsessively polite.

Well, yeah, as long as the solution works for you.
I didn't quite understand what was the original issue you were experiencing. Something like parsePhoneNumber() return type being incorrect?
Why doesn't the PhoneNumber return type work?

export function parsePhoneNumberFromString(
  text: string, 
  defaultCountry?: CountryCode | { 
    defaultCountry?: CountryCode, 
    defaultCallingCode?: string, 
    extract?: boolean 
  }
): PhoneNumber | undefined;

It seems like you're attempting to use the returned value as a key in a Map which I agree doesn't make any sense but why does it even allow it in the first place? It is an interface after all, not a string.

libphonenumber-js/types.d.ts

Lines 58 to 75 in 303f0f3

	export class PhoneNumber {
	constructor(countryCallingCodeOrCountry: CountryCallingCode | CountryCode, nationalNumber: NationalNumber, metadata: Metadata);
	countryCallingCode: CountryCallingCode;
	country?: CountryCode;
	nationalNumber: NationalNumber;
	number: E164Number;
	carrierCode?: CarrierCode;
	ext?: Extension;
	isPossible(): boolean;
	isValid(): boolean;
	getType(): NumberType;
	format(format: NumberFormat, options?: FormatNumberOptions): string;
	formatNational(options?: FormatNumberOptionsWithoutIDD): string;
	formatInternational(options?: FormatNumberOptionsWithoutIDD): string;
	getURI(options?: FormatNumberOptionsWithoutIDD): string;
	isNonGeographic(): boolean;
	isEqual(phoneNumber: PhoneNumber): boolean;
	}

[javascripter]

@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

[catamphetamine]

@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: #170 (comment)

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.

[javascripter]

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!

[tduyduc]

Is it possible to use type E164Number = string & {}; instead of interface E164Number extends String { }?

The variables s1 and s2 will be typed nominally (String1 and String2), but there is a caveat: (string & {}) extends object, so a type assertion (or a variable with string type) might still be necessary.

[catamphetamine]

FYI: I'm about to merge this new pull request that replaces the declaration for all "basic string" types like PhoneCode, Extension, etc.

Was:

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 { }

Will become:

type Tagged<A, T> = A & { __tag: T };

type E164Number = Tagged<string, "E164Number">;
type NationalNumber = Tagged<string, "NationalNumber">;
type Extension = Tagged<string, "Extension">;
type CarrierCode = Tagged<string, "CarrierCode">;
type CountryCallingCode = Tagged<string, "CountryCallingCode">;

If there're any objections then post them here.
I'll wait for a couple of days before merging that pull request.

[tduyduc]

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]

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.

[catamphetamine]

Published libphonenumber-js@1.9.48.

[atsu85]

@catamphetamine, cool workaround!
However i have one proposal for change, since as i understand the change only involves type definitions, not values, then instead of this:

type Tagged<A, T> = A & { __tag: T };

better option would be this:

type Tagged<A, T> = A & { __tag?: T };

as former doesn't give TS compiler error when trying to assign __tag to string

var e164: E164Number = "e164" as any
var tag: string = nationalNumber.__tag

but latter would, as expeted, because __tag will always be undefined instead of non-null/undefined string