fix: validator infinite loops

[christophehurpeau]

fixes #44

use a ref to keep the validator without impacting registerField

[ecrofeg]

@christophehurpeau looking forward for this fix

[apanizo]

Hi! @christophehurpeau good job, thanks for the PR.

If you want me to update the useField.test.js for covering this error, let me know. If you want to make the process faster just change the line 224:

const firstName = useField(FIELD_NAME, form, value => required(value))

In this way, you force sending a "different" fn on every render. Just checked your solution and it works!

[codecov]

Codecov Report

Merging #46 into master will not change coverage.
The diff coverage is 100%.

@@          Coverage Diff          @@
##           master    #46   +/-   ##
=====================================
  Coverage     100%   100%           
=====================================
  Files           4      4           
  Lines          81     83    +2     
  Branches       17     17           
=====================================
+ Hits           81     83    +2

Impacted Files	Coverage Δ
src/useField.js	100% <100%> (ø)	⬆️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f025858...878e460. Read the comment docs.

[christophehurpeau]

@apanizo thank you for the review :) test updated !

[apanizo]

So, one thing I have just tested, if you keep the validate variable here, the problem will appear again, and that makes sense. So I tested removing it, and then just use: getValidator: () => validate again and tests are green.

So what is the benefit of using the useRef in this case? As far as I understand useRef is good for keeping variables accessible inside the component (for example the interval id), but if at the end we are updating the pointer on each render (because validate is different), at the end, we are getting same results, doing more coding, right?

Unless I am wrong the real fix is remove validate not the useRef, thoughts?

[christophehurpeau]

If you only remove validate and if validate changes, it will be the first validate that always will be called, not the last changed. However I'm not sure if it's something that can happen often ?

[apanizo]

the first validate that always will be called

Interesting, what I have done in the same test you have just updated in this PR is:

const firstName = useField(FIELD_NAME, form, value => {
    console.log(randomString(5)) <--- This should be the same, right?
    return required(value)
})

And then, called:

const nameInput = container.querySelector('input')
TestUtils.Simulate.change(nameInput, { target: { value: 'f' } })
TestUtils.Simulate.change(nameInput, { target: { value: 'fo' } })
TestUtils.Simulate.change(nameInput, { target: { value: 'foo' } })

And for each change, the random string is different, so, what do you mean by "the first validate that always be called"? I thought you mean the validate's reference was stored/memoized, because the useEffect's content was not updated (because validate was not included in the useEffect's array).

But the previous snippet of code demonstrates my guess was not correct, and that makes sense, because normally when you change the input, the component where the useField lives is re-rendered, making the useEffect to be executed/updated again.

So... what am I missing?

Thanks for being part of this conversation!

[christophehurpeau]

I think randomString should always be different because it is called when validator() is called, not when validator is created.

I haven't tested, but it should be something like:

const randomStringValue = randomString(5);
const firstName = useField(FIELD_NAME, form, value => {
    console.log(randomStringValue) <--- This should be the same, because it uses randomStringValue from the first render
    return required(value)
})

useEffect is only called when name, form or subscription props changed (or on mount) and I think the point is to only register the field when this props changes :)

What do you think ? Thanks for this conversation too :)

[erikras]

Published in v2.0.1.

[lock]

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.