Skip to content

fix: validator infinite loops #46

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.

Sign up for GitHub

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

Merged
merged 2 commits into from
Jul 12, 2019
Merged

fix: validator infinite loops #46

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
3d7fe9c
fix: validator infinite loops
christophehurpeau May 20, 2019
878e460
test: update useField test
christophehurpeau May 31, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 6 additions & 3 deletions src/useField.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
import { useState, useEffect, useRef } from 'react'
import { fieldSubscriptionItems } from 'final-form'
import { useEffect, useRef, useState } from 'react'

export const all = fieldSubscriptionItems.reduce((result, key) => {
result[key] = true
Expand All @@ -21,8 +21,11 @@ const eventValue = event => {

const useField = (name, form, validate, subscription = all) => {
const autoFocus = useRef(false)
const validatorRef = useRef(undefined)
const [state, setState] = useState({})

validatorRef.current = validate

const deps = subscriptionToInputs(subscription)
useEffect(
() =>
Expand All @@ -38,12 +41,12 @@ const useField = (name, form, validate, subscription = all) => {
subscription,
validate
? {
getValidator: () => validate
getValidator: () => validatorRef.current
}
: undefined
),
// eslint-disable-next-line react-hooks/exhaustive-deps
[name, form, validate, ...deps]
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 ?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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!

Copy link
Contributor Author

@christophehurpeau christophehurpeau Jun 2, 2019 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

[name, form, ...deps]
)
let { blur, change, focus, value, ...meta } = state
delete meta.name // it's in input
Expand Down
2 changes: 1 addition & 1 deletion src/useField.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -221,7 +221,7 @@ describe('useField()', () => {
onSubmit,
validate
})
const firstName = useField(FIELD_NAME, form, required)
const firstName = useField(FIELD_NAME, form, value => required(value))

return (
<form onSubmit={handleSubmit}>
Expand Down