Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
tamonmon0417 opened this issue Mar 4, 2020 · 10 comments
Open
Assignees
Labels

Comments

@tamonmon0417
Copy link

tamonmon0417 commented Mar 4, 2020

Steps to reproduce

return AsYouType().input(null)

Observed result

TypeError: Cannot read property 'search' of null

Expected result

return null

@nshCore
Copy link

nshCore commented Mar 4, 2020

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
Copy link
Owner

@nshCore is correct. Closing.

@catamphetamine catamphetamine reopened this Mar 5, 2020
@catamphetamine catamphetamine changed the title AsYouType().input(text) causes error when text is null. (Breaking change) AsYouType().input(null) no longer works Mar 5, 2020
@catamphetamine
Copy link
Owner

catamphetamine commented Mar 5, 2020

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:
b66012d#diff-0adfb6ac49dce262c372f3929c845c3e
The version with the change seems to be 1.7.32.

@catamphetamine catamphetamine changed the title (Breaking change) AsYouType().input(null) no longer works (Breaking change in 1.7.32) AsYouType().input(null) no longer works Mar 5, 2020
@tamonmon0417
Copy link
Author

tamonmon0417 commented Mar 6, 2020

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
Copy link
Owner

@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
Copy link

jbberinger commented Mar 23, 2020

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
Copy link
Owner

@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
Copy link
Owner

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
Copy link

@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
Copy link
Owner

catamphetamine commented Mar 23, 2020

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

4 participants