(Breaking change in 1.7.32) AsYouType().input(null) no longer works

[tamonmon0417]

Steps to reproduce

return AsYouType().input(null)

Observed result

TypeError: Cannot read property 'search' of null

Expected result

return null

[nshCore]

AsYouType().input() expects a string not null

	/**
	 * Inputs "next" phone number characters.
	 * @param  {string} text
	 * @return {string} Formatted phone number characters that have been input so far.
	 */
	input(text) {
		const formattedDigits = this.extractFormattedDigits(text)
		// If the extracted phone number part
		// can possibly be a part of some valid phone number
		// then parse phone number characters from a formatted phone number.
		if (VALID_FORMATTED_PHONE_NUMBER_PART_PATTERN.test(formattedDigits)) {
			this.formattedOutput = this.getFullNumber(
				this.inputDigits(parseDigits(formattedDigits)) ||
				this.getNonFormattedNationalNumber()
			)
		}
		return this.formattedOutput
	}

you can't search null as null is not typeof string.

[catamphetamine]

@nshCore is correct. Closing.

[catamphetamine]

There has been another issue about this change, so I'll reopen this issue so that others could see it. While this is indeed a breaking change, I think no one should have been passing null there in the first place. So I don't consider this an issue, even if it breaks someone's code.

Added a note in the changelog.

The relevant change seems to be this big refactoring of AsYouType.js: https://github.com/catamphetamine/libphonenumber-js/commit/b66012dc70be42e3ecb3ae8863459be945a1034a#diff-0adfb6ac49dce262c372f3929c845c3e The version with the change seems to be 1.7.32.

[tamonmon0417]

May I know why you don't handle text like below?

function extractFormattedPhoneNumber(text) {
        if (!text) {
          ...
        }
        // Attempt to extract a possible number from the string passed in.
	const startsAt = text.search(VALID_PHONE_NUMBER)
	if (startsAt < 0) {
		return
	}

I think the adding of extractFormattedPhoneNumber function is not a patch level change. It produce uncovered errors to users.

[catamphetamine]

@tamonmon0417

It produce uncovered errors to users.

I did agree that it does break some code. But, it only breaks the code that used to pass null to AsYouType.input(), which doesn't make any sense. I don't think there could be any valid use case when a developer might pass null to it.

[jbberinger]

I agree with @tamonmon0417 about this being more than a patch level change. Ran into this bug today after upgrading from 1.7.28 -> 1.7.48. If a value is null or undefined, a sanity check is necessary now.

[catamphetamine]

@jbberinger

If a value is null or undefined, a sanity check is necessary now.

Yes, this results in an extra if condition. This condition could be inside the library or outside of it. I decided to move it outside just to make the function more strict. The reasoning is: "If there's nothing to format, then it shouldn't be formatted at all". This results in "one-liners" like new AsYouType().input(user. phone) not working in cases when a user doesn't have a phone.

[catamphetamine]

Commenting on the previous comment, the better way would be creating a custom "helper" function in an app, something like formatNumber(), where it would have the if condition inside, and then such function would be imported in the application code, so that the application doesn't use AsYouType directly.

[jbberinger]

@catamphetamine I agree that it's entirely up to you to decide the types your library's function's will accept/handle, and that it makes no sense to attempt to format a null value. I think the only issue anyone would have is the fact that this is a case of subtracting functionality (ie. introducing breaking changes) in a patch. It's an easy enough fix on my end, but I'm sure there will be more people running into this as they upgrade.

[catamphetamine]

@jbberinger Yes, this is certainly violating the SemVer spec )