Skip to content
This repository has been archived by the owner on Apr 6, 2023. It is now read-only.

Use of useRef is incorrect #339

Open
kkirby opened this issue Jan 12, 2021 · 6 comments
Open

Use of useRef is incorrect #339

kkirby opened this issue Jan 12, 2021 · 6 comments

Comments

@kkirby
Copy link

kkirby commented Jan 12, 2021

I noticed that the use of references is incorrect, at least in the way that I believe the library is meant to be used.

When calling registerField, you are passing a ref's value (ref.current) as opposed to the ref. This means that if the ref ever updates, unform will never see the update. This is because you are passing the value of the ref and not the ref itself. In order to maintain a guaranteed connection with the field, you must accept the ref, not the value.

A simple workaround is to pass the ref's value to the useEffect that calls registerField, though I don't know if calling registerField multiple times for the same field results in expected behavior.

At any rate, I've updated the playground to show an example of how passing the value of the ref can break things:

Sandbox

To test:

  1. Type some data into the First Name field.
  2. Click on Toggle First Name to remove the input.
  3. Then click the button again to bring it back.
  4. Type some more data into First Name
  5. Click Reset

Observe that the field does not reset, and if you click save, the value does not appear.

@carlosallexandre
Copy link

carlosallexandre commented Jan 16, 2021

Hey @kkirby,

I got the problem. It could be solved in many ways. But, trying somethings, i got this simple solution if you need:

useEffect(() => {
    registerField({
      name: fieldName,
      ref: inputRef,
      path: "current.value",
      clearValue: (ref) => {
        ref = "";
      }
    });
  }, [fieldName, registerField]);

Sandbox forked from yours

Tell me if it worked for you.

@kkirby
Copy link
Author

kkirby commented Jan 21, 2021

@carlosallexandre Your solution isn't really a fix to this issue. It's more of a workaround.

@carlosallexandre
Copy link

carlosallexandre commented Jan 21, 2021

@kkirby

Indeed, i agree. But let's see some points:

if u toggle your input outside of component (really unmount) you see it works properly (without "workaround"). Why? Because unregisterField, called inside useField when component is unmounted, will remove the inputRef from the array where it stored, so this way when mount again the inputRef will be updated.

In your use case you don't unmount the entirely component and a change on inputRef.current doesn't dispatch a re-render. So, even if you put inputRef.current on useEffect deps it doesn't work. Another "workaround", it would be put the active state (that toggle the component) on deps. Trouble with this "workaround", the old inputRef isn't removed from the form like implemented over unform todays, because again, unregisterFied isn't called.

So for solutions could be done:

  • expose unregisterField;
  • or change the way like the inputRefs is stored for a object using the name as key;
  • or expose a callback ref from useField;

@diego3g
Copy link
Contributor

diego3g commented Jan 21, 2021

@kkirby Thanks for the contribution here, all that you said make a lot of sense to me, i'll take a deeper look into it and possibly make a PR.

diego3g added a commit that referenced this issue Jan 27, 2021
Currently we used to pass the ref.current value to registerField
function, but with that Unform was referencing the current value
and no the reference itself and sometimes we were losing track
of the reference to get the field value.

Fixes #339.
diego3g added a commit that referenced this issue Jan 27, 2021
Currently we used to pass the ref.current value to registerField
function, but with that Unform was referencing the current value
and no the reference itself and sometimes we were losing track
of the reference to get the field value.

Fixes #339.
diego3g added a commit that referenced this issue Jan 27, 2021
Currently we used to pass the ref.current value to registerField
function, but with that Unform was referencing the current value
and no the reference itself and sometimes we were losing track
of the reference to get the field value.

Fixes #339.
diego3g added a commit that referenced this issue Jan 28, 2021
Currently we used to pass the ref.current value to registerField
function, but with that Unform was referencing the current value
and no the reference itself and sometimes we were losing track
of the reference to get the field value.

Fixes #339.
@vjee
Copy link

vjee commented Mar 8, 2021

Any progress on this? It looks like the PR is ready but not yet merged.

@jpedroschmitz
Copy link
Member

Hey @vjee, this will probably lead to breaking changes, so we are waiting to release this new version.

Probably going to release an alpha 3.0 version soon.

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

Successfully merging a pull request may close this issue.

5 participants