jackocnr / intl-tel-input

A JavaScript plugin for entering and validating international telephone numbers
https://intl-tel-input.com
MIT License
7.38k stars 1.93k forks source link

React component - allow access to intlTelInput instance #1619

Closed BRAiNCHiLD95 closed 1 month ago

BRAiNCHiLD95 commented 2 months ago

At the moment, it isn't possible to interact with the underlying intlTelInput instance when using the React version of this plugin.

Plugin version

v23.0.4

Steps to reproduce

  1. Use React component (sample usage - https://codepen.io/BRAiNCHiLD95/pen/MWdaGrX)
  2. Try to add number on button click (e.g. like a "quick add" contact feature).
  3. Since the component is uncontrolled (only takes in defaultValue), the button click never works to update the input field itself.

Expected behaviour

Should be able to add/set number on button click (i.e. dynamically) using something like itiRef.current.setNumber()

Actual behaviour

The validator seems to reject the number directly, and it never shows up on the input field at all.

Initialisation options

{
    initialCountry: "us",
    separateDialCode: true,
    // utilsScript
}

Codepen: https://codepen.io/BRAiNCHiLD95/pen/MWdaGrX

Additional Notes:

Since the element has a ref (used internally), passing a ref to inputProps actually breaks important functionality.

Maybe an optional instanceRef prop could be used to expose internal functions in React? Could be as simple as re-assigning itiRef to the instanceRef (if passed in).

Code: ``` useEffect(() => { // store a reference to the current input ref, which otherwise is already lost in the cleanup function const inputRefCurrent = inputRef.current; if (inputRefCurrent) { itiRef.current = intlTelInput(inputRefCurrent, initOptions); if (inputProps && inputProps.instanceRef) { // assign value if prop available (maybe perform additional checks?) inputProps.instanceRef.current = itiRef.current; } inputRefCurrent.addEventListener("countrychange", update); // when plugin initialisation has finished (e.g. loaded utils script), update all the state values itiRef.current.promise.then(update); } return (): void => { if (inputRefCurrent) { inputRefCurrent.removeEventListener("countrychange", update); } itiRef.current?.destroy(); if (inputProps && inputProps.instanceRef) { inputProps.instanceRef.current = null; // hopefully destroying twice is not necessary. } }; }, []); ```
jackocnr commented 2 months ago

Thanks for raising this. I've tried exposing the instance and input refs to the parent in v23.0.6. So now you should be able to do something like this:

const ref = useRef(null);
return (
    <IntlTelInput ref={ref} />
    <button onClick={ref.current.getInstance().setNumber(...)}>Go</button>
);

or ref.current.getInput() if you need the input ref.

Let me know what you think.

BRAiNCHiLD95 commented 2 months ago

That's actually perfect! I thought the underlying instance would be enough, but you were right to get the input as well - the inputRef is what I needed.

Had another small hiccup - Since all events are on the input element, triggering a custom "input" event seems like the only way to get the rest of the functionality intact (i.e. validity and number change handlers firing, etc.)

React needed a workaround for it.

In the end, this worked perfectly -

// simplified example of the onClick handler
const quickAddNumber = () => {
  const itiInput = intlTelInputRef.current.getInput();
  const nativeInputValueSetter = Object.getOwnPropertyDescriptor(window.HTMLInputElement.prototype,"value").set;
  nativeInputValueSetter.call(itiInput, "+12015550123");
  const event = new Event("input", { bubbles: true });
  itiInput.dispatchEvent(event);
}

I've updated my codepen with a slighly more updated example for anyone interested. It works as expected.

BRAiNCHiLD95 commented 2 months ago

Additional note for anyone interested -

In the codepen, I'm simply using random index number to get a number from an array of numbers. So the incoming value was pretty much always different - leading to a proper "change/input" being detected. But in my own codebase, the number is fixed (and can be removed + re-added).

I faced an issue where the underlying input tag was holding onto the older value because I was simply using setNumber(null) to reset things - obviously the better and more accurate way to do this now is to use the getInput() function, and to use the native input setter bypass for react to set the value instead.

To put some images to this logic, the intended UI/UX is something like -

image

Click on the button does - validation + creates a tag out of the number

image

We use tagify for that tag UX. So 'removing' the tag would re-enable the button and start the cycle again.

jackocnr commented 2 months ago

Thanks for sharing all of this.

Regarding all the custom code for triggering an input event, do you think it would make sense / be possible for me to re-order things inside the react component so this wasn't necessary? Currently, we trigger all the internal updates (invoking onChangeNumber etc) based on the React <input> onInput prop, but what if we removed this, and instead setup a custom "input" event listener on the (DOM) input element directly (e.g. inputRef.current.addEventListener("input", ...))? The idea being that you could then just use getInstance().setNumber(...) and it would trigger all the internal updates automatically - what do you think?

BRAiNCHiLD95 commented 2 months ago

I believe the approach you suggested is more in line with the existing VanillaJS implementation, isn't it? (i.e. using the instance methods for dynamic access)

Yes - that would be the better approach, 100%. Because then react/nextjs devs aren't going too far off from the base implementation and docs - and it possibly also makes it a little less annoying for you to continue maintaining it?


PS: If your internal implementation does need to use the React-compatible way of updating a value -

// get the input element and newValue from wherever and then -

const nativeInputValueSetter = Object.getOwnPropertyDescriptor(
    window.HTMLInputElement.prototype,
    'value'
).set;

nativeInputValueSetter.call(inputElem, newValue);
jackocnr commented 2 months ago

If you upgrade to v23.0.7 you should be able to use getInstance().setNumber(...) and have it automatically trigger the internal update calls. Let me know what you think.

BRAiNCHiLD95 commented 1 month ago

Since you are using useImperativeHandle - instead of having to override each method (assuming there's a possibility of more instance functions misbehaving/needing an override).. do you think it would make more sense to have the Iti instance stored as a state variable instead of a ref?

And that could be in the dependency array for useImperativeHandle, making sure you always have access to the latest version? It might work better than a ref since you can't use those for dependencies.

jackocnr commented 1 month ago

It's an interesting idea, but then how would you override the methods to add the update call? (I'd be open to a pull request if you think you could get this working)

BRAiNCHiLD95 commented 1 month ago

@jackocnr I've been working on this and I think I've figured out what the roadblock is right now. Correct me if any of this is wrong.

From what I've understood, you have a custom event for countrychange, that essentially calls the update function. I do not see any other usage of countrychange other than in the readme (meaning this is actually exposed to devs, by default).

Obviously - this only runs when the country changes - which means that if the country code is the same - instance.setNumber() does not trigger any events, by default.

Would you be open to having a custom change event instead? Something that could be triggered when either the number or the country changes?

OR maybe a numberchange event incase refactoring countrychange could break functionality for other projects? i.e. a change event that is triggered when instance.setNumber() is called?

Instead of extending the instance's functionality inside useImperativeHandle - maybe the API itself could expose a separate event

I'll just share a copy of what I've got working right now. To give you some highlights, here's what was changed -

What do you think?

CODE: react.tsx (function contents) ``` const inputRef = useRef(null); const [itiInstance, setItiInstance] = useState(null); const update = useCallback((e): void => { console.log("update called", e, itiInstance); const num = itiInstance?.getNumber() || ""; const countryIso = itiInstance?.getSelectedCountryData().iso2 || ""; // note: this number will be in standard E164 format, but any container component can use // intlTelInput.utils.formatNumber() to convert this to another format // as well as intlTelInput.utils.getNumberType() etc. if need be onChangeNumber(num); onChangeCountry(countryIso); if (itiInstance) { const isValid = usePreciseValidation ? itiInstance.isValidNumberPrecise() : itiInstance.isValidNumber(); if (isValid) { onChangeValidity(true); onChangeErrorCode(null); } else { const errorCode = itiInstance.getValidationError(); onChangeValidity(false); onChangeErrorCode(errorCode); } } }, [itiInstance, onChangeValidity, onChangeErrorCode]); const updateNumber = useCallback((e) => { if (itiInstance) { itiInstance.setNumber(e.target.value || ""); } }, [itiInstance]); // expose the instance and input ref to the parent component useImperativeHandle(ref, () => ({ getInstance: () => itiInstance, getInput: () => inputRef.current, }), [itiInstance, update]); useEffect(() => { // store a reference to the current input ref, which otherwise is already lost in the cleanup function const inputRefCurrent = inputRef.current; if (inputRefCurrent) { setItiInstance(intlTelInput(inputRefCurrent, initOptions)); } return (): void => { setItiInstance((prevInstance) => { if (prevInstance) prevInstance.destroy(); return null; }); }; }, []); useEffect(() => { if (itiInstance && inputRef.current) { inputRef.current.addEventListener("input", updateNumber); inputRef.current.addEventListener("countrychange", update); // when plugin initialisation has finished (e.g. loaded utils script), update all the state values itiInstance.promise.then(update); return () => { inputRef.current.removeEventListener("input", updateNumber); inputRef.current.removeEventListener("countrychange", update); }; } }, [itiInstance, update, updateNumber]); return ( ); ```
jackocnr commented 1 month ago

Thanks for looking into this.

I've just released v23.0.9 which triggers an "input" event on setNumber, so the normal react onInput handler catches this and triggers an update. Give it a try and let me know what you think.

I'm interested in all of your other changes, e.g. the dependencies on useImperativeHandle and useEffect, switching to useState, useCallback etc, but would it be possible for you to describe (or ideally show with an example) why all of these changes are needed e.g. in what situation will it break if we don't make these changes?

BRAiNCHiLD95 commented 1 month ago

I've just released v23.0.9 which triggers an "input" event on setNumber, so the normal react onInput handler catches this and triggers an update. Give it a try and let me know what you think.

Sure! I'll try it out!

I'm interested in all of your other changes, e.g. the dependencies on useImperativeHandle and useEffect, switching to useState, useCallback etc, but would it be possible for you to describe (or ideally show with an example) why all of these changes are needed e.g. in what situation will it break if we don't make these changes?

Overall - some (if not all) of these aspects help make a more robust React component that follows the best practices. Obviously the useRef approach is a 100% valid. But it doesn't support side-effects/reactivity - and if the package needs side-effects (i.e. for certain operations to run once the instance is created, useState is better).

The only possible cases where something would break in the current implementation is when there are multiple async operations running in the parent component that add delays in the complete initialization of the intl-tel-input component - I'm not sure how to simulate that in an isolated codepen environment though.

To try and break down my reasoning -

  1. since useRef cannot be used for side-effects, using useState along with useEffect ensures we can actually keep the component state consistent with the UI - functions/side-effects that rely on the instance being available, will always have access to it.
  2. Separate useEffect's give us more control over when those event listeners are attached (I would split change and country change across 2 separate listeners, ideally) - and since those listeners themselves can have more dependencies, it helps ensure you are attaching the most updated ones. Also - there's no real reason to attach listeners without the existence of the Iti instance - is there?
  3. The absence of the instance could be potentially be used to conditionally render a loading state OR even for error handling - for e.g. utils script failing to load could be used to destroy the instance and retry (or simply notify devs of the critical error)?
  4. The useCallbacks are simply to reduce re-renders and/or to ensure the latest values are accessible at all times - for e.g., when instance is updated, the updateNumber function changes - so a new re-assignment of the event listener is needed.
  5. The dependencies is useImperativeHandle are also purely for ensuring that the usage of that hook always provides the latest instance value.
BRAiNCHiLD95 commented 1 month ago

@jackocnr the input trigger works perfectly, btw!

jackocnr commented 1 month ago

Thanks for the write-up.

I've just released v23.0.10, where I've adopted some of your suggestions, including useCallback with deps, splitting up useEffect and adding a dep in one case.

I haven't switched to useState for the plugin instance object, as for me, this component is completely tied to the instance - it should be created when the component is first added and destroyed when the component is removed, and that's it - it should not change otherwise - it should not be destroyed and re-created while the component is in use - that would result in a UI flash and the plugin is not designed to handle that anyway - likely it would not behave as expected (e.g. selected country would not be persisted), so I don't want to take any steps towards supporting this just yet - it would be a much bigger change.

Let me know if there's anything else you think it's important we change right now, else I'll close this issue.

Thanks again for your help on this.

BRAiNCHiLD95 commented 1 month ago

Makes sense!

I saw the changes - looks good to me!

We can close this now - the core issue has been resolved for sure.