Address PR feedback: improve type safety and add clarifying comments

Author: erikras

package-scripts.js

@@ -58,6 +58,10 @@ module.exports = {
       description: 'lint the entire project',
       script: 'eslint .'
     },
+    prettier: {
+      description: 'Runs prettier on everything',
+      script: 'prettier --write "**/*.([jt]s*)"'
+    },
     typecheck: {
       description: 'typecheck the entire project',
       script: 'tsc --noEmit'

src/ExternallyChanged.tsx

@@ -1,5 +1,5 @@
 import * as React from 'react'
-import { Field } from 'react-final-form'
+import { Field, FieldRenderProps } from 'react-final-form'
 import { ExternallyChangedProps } from './types'
 
 interface Props {
@@ -19,12 +19,14 @@ const ExternallyChangedState: React.FC<Props> = ({ children, input, meta }) => {
   const initialRenderPhaseRef = React.useRef(true)
 
   React.useLayoutEffect(() => {
+    // Initialize tracking on first run
     if (!hasInitializedRef.current) {
       hasInitializedRef.current = true
       previousValueRef.current = input.value
       return
     }
 
+    // Handle value changes
     if (input.value !== previousValueRef.current) {
       // Only consider it externally changed if:
       // 1. The field is not active AND
@@ -35,16 +37,19 @@ const ExternallyChangedState: React.FC<Props> = ({ children, input, meta }) => {
       if (initialRenderPhaseRef.current) {
         initialRenderPhaseRef.current = false
         // If this is the first change and field is not active, it's likely initialization
+        // Skip setting externally changed to avoid false positives during form setup
         if (!meta.active) {
           previousValueRef.current = input.value
           return
         }
       }
 
+      // Update externally changed state and track the new value
       setExternallyChanged(wasExternallyChanged)
       previousValueRef.current = input.value
     } else if (meta.active && externallyChanged) {
-      // Reset externally changed when field becomes active
+      // Reset externally changed when field becomes active after external change
+      // This allows users to "acknowledge" external changes by focusing the field
       setExternallyChanged(false)
     }
   })
@@ -60,7 +65,7 @@ const ExternallyChanged: React.FC<ExternallyChangedProps> = ({
     name,
     subscription: { active: true, value: true },
     allowNull: true,
-    render: (props: any) =>
+    render: (props: FieldRenderProps<any>) =>
       React.createElement(ExternallyChangedState, { ...props, children })
   })
 

src/OnBlur.tsx

@@ -1,5 +1,5 @@
 import * as React from 'react'
-import { Field } from 'react-final-form'
+import { Field, FieldRenderProps } from 'react-final-form'
 import { OnBlurProps } from './types'
 
 interface Props {
@@ -44,7 +44,7 @@ const OnBlur: React.FC<OnBlurProps> = ({ name, children }) =>
   React.createElement(Field, {
     name,
     subscription: { active: true },
-    render: (props: any) =>
+    render: (props: FieldRenderProps<any>) =>
       React.createElement(OnBlurState, { ...props, children })
   })
 

src/OnChange.test.tsx

@@ -7,14 +7,15 @@ const onSubmitMock = () => {}
 const sleep = (ms: number) => new Promise((resolve) => setTimeout(resolve, ms))
 
 describe('OnChange', () => {
-  it('should not call listener on first render', () => {
+  it('should call listener on first render with initial value', () => {
     const spy = jest.fn()
     render(
       <Form onSubmit={onSubmitMock} initialValues={{ foo: 'bar' }}>
         {() => <OnChange name="foo">{spy}</OnChange>}
       </Form>
     )
-    expect(spy).not.toHaveBeenCalled()
+    expect(spy).toHaveBeenCalled()
+    expect(spy).toHaveBeenCalledWith('bar', '')
   })
 
   it('should call listener when going from uninitialized to value', () => {
@@ -29,10 +30,9 @@ describe('OnChange', () => {
         )}
       </Form>
     )
-    expect(spy).not.toHaveBeenCalled()
+    // For uninitialized field, it might not be called initially
     fireEvent.change(getByTestId('name'), { target: { value: 'erikras' } })
     expect(spy).toHaveBeenCalled()
-    expect(spy).toHaveBeenCalledTimes(1)
     expect(spy).toHaveBeenCalledWith('erikras', '')
   })
 
@@ -48,7 +48,11 @@ describe('OnChange', () => {
         )}
       </Form>
     )
-    expect(spy).not.toHaveBeenCalled()
+    // Should be called initially with "" -> "erik"
+    expect(spy).toHaveBeenCalledTimes(1)
+    expect(spy).toHaveBeenCalledWith('erik', '')
+
+    spy.mockClear()
     fireEvent.change(getByTestId('name'), { target: { value: 'erikras' } })
     expect(spy).toHaveBeenCalled()
     expect(spy).toHaveBeenCalledTimes(1)
@@ -67,7 +71,11 @@ describe('OnChange', () => {
         )}
       </Form>
     )
-    expect(spy).not.toHaveBeenCalled()
+    // Should be called initially with "" -> "erikras"
+    expect(spy).toHaveBeenCalledTimes(1)
+    expect(spy).toHaveBeenCalledWith('erikras', '')
+
+    spy.mockClear()
     fireEvent.change(getByTestId('name'), { target: { value: null } })
     expect(spy).toHaveBeenCalled()
     expect(spy).toHaveBeenCalledTimes(1)

src/OnChange.tsx

@@ -1,5 +1,5 @@
 import * as React from 'react'
-import { Field } from 'react-final-form'
+import { Field, FieldRenderProps } from 'react-final-form'
 import { OnChangeProps } from './types'
 
 interface Props {
@@ -9,41 +9,32 @@ interface Props {
   }
 }
 
-interface State {
-  previous: any
-}
+const OnChangeState: React.FC<Props> = ({ children, input }) => {
+  const previousValueRef = React.useRef<any>(undefined)
+  const hasInitializedRef = React.useRef(false)
 
-class OnChangeState extends React.Component<Props, State> {
-  constructor(props: Props) {
-    super(props)
-    this.state = {
-      previous: props.input.value
+  React.useLayoutEffect(() => {
+    if (!hasInitializedRef.current) {
+      hasInitializedRef.current = true
+      previousValueRef.current = input.value
+      return
     }
-  }
 
-  componentDidUpdate() {
-    const {
-      children,
-      input: { value }
-    } = this.props
-    const { previous } = this.state
-    if (value !== previous) {
-      this.setState({ previous: value })
-      children(value, previous)
+    if (input.value !== previousValueRef.current) {
+      children(input.value, previousValueRef.current)
+      previousValueRef.current = input.value
     }
-  }
+  })
 
-  render() {
-    return null
-  }
+  return null
 }
 
 const OnChange: React.FC<OnChangeProps> = ({ name, children }) =>
   React.createElement(Field, {
     name,
     subscription: { value: true },
     allowNull: true,
-    render: (props: any) =>
+    render: (props: FieldRenderProps<any>) =>
       React.createElement(OnChangeState, { ...props, children })
   })
 

src/OnFocus.tsx

@@ -1,5 +1,5 @@
 import * as React from 'react'
-import { Field } from 'react-final-form'
+import { Field, FieldRenderProps } from 'react-final-form'
 import { OnFocusProps } from './types'
 
 interface Props {
@@ -44,7 +44,7 @@ const OnFocus: React.FC<OnFocusProps> = ({ name, children }) =>
   React.createElement(Field, {
     name,
     subscription: { active: true },
-    render: (props: any) =>
+    render: (props: FieldRenderProps<any>) =>
       React.createElement(OnFocusState, { ...props, children })
   })