Giter Site home page Giter Site logo

Comments (14)

jackocnr avatar jackocnr commented on June 3, 2024 1

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.

from intl-tel-input.

jackocnr avatar jackocnr commented on June 3, 2024 1

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)

from intl-tel-input.

BRAiNCHiLD95 avatar BRAiNCHiLD95 commented on June 3, 2024 1

@jackocnr the input trigger works perfectly, btw!

from intl-tel-input.

jackocnr avatar jackocnr commented on June 3, 2024 1

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.

from intl-tel-input.

BRAiNCHiLD95 avatar BRAiNCHiLD95 commented on June 3, 2024 1

Makes sense!

I saw the changes - looks good to me!

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

from intl-tel-input.

BRAiNCHiLD95 avatar BRAiNCHiLD95 commented on June 3, 2024

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.

from intl-tel-input.

BRAiNCHiLD95 avatar BRAiNCHiLD95 commented on June 3, 2024

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.

from intl-tel-input.

jackocnr avatar jackocnr commented on June 3, 2024

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?

from intl-tel-input.

BRAiNCHiLD95 avatar BRAiNCHiLD95 commented on June 3, 2024

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);

from intl-tel-input.

jackocnr avatar jackocnr commented on June 3, 2024

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.

from intl-tel-input.

BRAiNCHiLD95 avatar BRAiNCHiLD95 commented on June 3, 2024

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.

from intl-tel-input.

BRAiNCHiLD95 avatar BRAiNCHiLD95 commented on June 3, 2024

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

  • We use useState instead of useRef for the core instance itself - and we properly re-create and re-attach listeners as and when the instance is re-created.
  • useImperativeHandle also uses the instance as a dependency, so the ref passed in by the dev will always reference the current value of the instance.
  • useCallback is used for the functions to ensure they have access to the latest instance, etc..
  • We retain the onInput functionality by using a separate function that internally uses instance.setNumber() - this can make sure that all usage goes throught the same core logic.

What do you think?

CODE: react.tsx (function contents)
  const inputRef = useRef<HTMLInputElement | null>(null);
  const [itiInstance, setItiInstance] = useState<Iti | null>(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 (
    <input
      id='intl-tel-input-field'
      type="tel"
      ref={inputRef}
      defaultValue={initialValue}
      {...inputProps}
    />
  );

from intl-tel-input.

jackocnr avatar jackocnr commented on June 3, 2024

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?

from intl-tel-input.

BRAiNCHiLD95 avatar BRAiNCHiLD95 commented on June 3, 2024

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.

from intl-tel-input.

Related Issues (20)

Recommend Projects

  • React photo React

    A declarative, efficient, and flexible JavaScript library for building user interfaces.

  • Vue.js photo Vue.js

    🖖 Vue.js is a progressive, incrementally-adoptable JavaScript framework for building UI on the web.

  • Typescript photo Typescript

    TypeScript is a superset of JavaScript that compiles to clean JavaScript output.

  • TensorFlow photo TensorFlow

    An Open Source Machine Learning Framework for Everyone

  • Django photo Django

    The Web framework for perfectionists with deadlines.

  • D3 photo D3

    Bring data to life with SVG, Canvas and HTML. 📊📈🎉

Recommend Topics

  • javascript

    JavaScript (JS) is a lightweight interpreted programming language with first-class functions.

  • web

    Some thing interesting about web. New door for the world.

  • server

    A server is a program made to process requests and deliver data to clients.

  • Machine learning

    Machine learning is a way of modeling and interpreting data that allows a piece of software to respond intelligently.

  • Game

    Some thing interesting about game, make everyone happy.

Recommend Org

  • Facebook photo Facebook

    We are working to build community through open source technology. NB: members must have two-factor auth.

  • Microsoft photo Microsoft

    Open source projects and samples from Microsoft.

  • Google photo Google

    Google ❤️ Open Source for everyone.

  • D3 photo D3

    Data-Driven Documents codes.