input.onChange has a new reference each time value changes

[adrienharnay]

Are you submitting a bug report or a feature request?

Bug report

What is the current behavior?

Every time a field value changes, onChange gets a new reference (and maybe other event handlers do as well).

What is the expected behavior?

When building a custom field and using useEffect, it's frequent to depend on onChange to update a value. In my case for a large field, I use useMemo to make sure I only rerender the parts that need it, but onChange makes sure everything gets updated every time the value changes. Is there another way than useCallback(input.onChange, [])?

Sandbox Link

https://codesandbox.io/s/react-final-form-third-party-components-example-lo4yo?file=/index.js

What's your environment?

(Latest on every lib, as seen on the sandbox)

Other information

/

Thanks in advance!

[afzalsayed96]

The issue lies here https://github.com/final-form/react-final-form/blob/master/src/useField.js#L184 (useCallback subscribing to state.values)
@erikras any suggestions how we could fix this?

[kapral18]

This hits a pretty interesting topic about what should affect public API method identity.

There is definitely a (blunt) way to solve this problem by moving all dependencies into mutable vars/refs

  const _valueRef = React.useRef(_value)
  React.useEffect(() => {
    _valueRef.current = _value
  })

  const stateRef = React.useRef(state)
  React.useEffect(() => {
    stateRef.current = state
  })
  
  const nameRef = React.useRef(name)
  React.useEffect(() => {
    nameRef.current = name
  })
  
  const typeRef = React.useRef(type)
  React.useEffect(() => {
    typeRef.current = type
  })
  
  // etc...
  //...
  
    onChange: React.useCallback(
      (event: SyntheticInputEvent<*> | any) => {
        //...
        const value: any =
          event && event.target
            ? getValue(event, stateRef.current.value, _valueRef.current, isReactNative)
            : event
        stateRef.current.change(parseRef.current(value, nameRef.current))
      },
      []
    ),
    onFocus: React.useCallback(
      (event: ?SyntheticFocusEvent<*>) => {
        stateRef.current.focus()
      },
      []
    )

Thus we don't need to depend on any direct reference from closure.

But this will break a test introduced by #787 https://github.com/final-form/react-final-form/pull/787/files#diff-2824eafc0ee519762539c9b997ab9f64d71babadb83e52ec1cf2d7e5ef0e40ccR177 requiring the identities of functions to change on every config.name change, which doesn't make much sense to me, given that the problem it was trying to solve, i.e. fix the stale closure problem, is absent with aforementioned approach.

But this means we have to always put explicit dependencies in mutable vars/refs which might not be the most aesthetic thing to do.

On the other hand we guarantee that identity of the methods are stable from the very start, the same way react guarantees that dispatch or setState identity is preserved throughout rerenders.

Unstable method identity is unnecessary and annoying but I am myself conflicted about the solution above so wdyt? (cc: @erikras)

[erikras]

we guarantee that identity of the methods are stable from the very start, the same way react guarantees that dispatch or setState identity is preserved throughout rerenders.

That's the winning argument, I think. Callbacks a library gives you should be instance-stable across renders.

This will be a breaking change. @kapral18, can you PR this change?

[erikras]

Here's a test I wrote when investigating this. Please include it in the PR.

  it('should give same instance of handlers as value changes', () => {
    const spy = jest.fn()
    const MyField = ({ name }) => {
      const { input } = useField(name, { subscription: { value: true } })
      const { onChange, onFocus, onBlur } = input
      spy(onChange, onFocus, onBlur)
      return <input {...input} />
    }
    render(
      <Form onSubmit={onSubmitMock}>
        {() => (
          <form>
            <MyField name="myField" />
          </form>
        )}
      </Form>
    )

    expect(spy).toHaveBeenCalledTimes(2)
    const [onChange, onFocus, onBlur] = spy.mock.calls[1]
    const setValue = value => {
      act(() => {
        onFocus()
        onChange(value)
        onBlur()
      })
    }
    setValue('dog')
    expect(spy).toHaveBeenCalledTimes(3)
    expect(spy.mock.calls[2][0]).toBe(spy.mock.calls[1][0]) // onChange
    expect(spy.mock.calls[2][1]).toBe(spy.mock.calls[1][1]) // onFocus
    expect(spy.mock.calls[2][2]).toBe(spy.mock.calls[1][2]) // onBlur
    setValue('cat')
    expect(spy).toHaveBeenCalledTimes(4)
    expect(spy.mock.calls[3][0]).toBe(spy.mock.calls[1][0]) // onChange
    expect(spy.mock.calls[3][1]).toBe(spy.mock.calls[1][1]) // onFocus
    expect(spy.mock.calls[3][2]).toBe(spy.mock.calls[1][2]) // onBlur
    expect(spy.mock.calls[3][0]).toBe(spy.mock.calls[2][0]) // onChange
    expect(spy.mock.calls[3][1]).toBe(spy.mock.calls[2][1]) // onFocus
    expect(spy.mock.calls[3][2]).toBe(spy.mock.calls[2][2]) // onBlur
  })

[erikras]

Published fix in v6.5.4.