Repository: final-form/react-final-form (js)

Issue:
Should the Components really extend PureComponent?
Hey, really awesome library, thanks so much for sharing!

# tl;dr

I think that all Components that use render props should extend `Component`, instead of `PureComponent`.

# Longer version

My colleague noticed that the `<Form>` component is a `PureComponent`, so he thought it'd be a good idea to maintain referential identity between renders, and define the render function in such a way that it would allow `<Form>` to opt-out of rerendering. Like so:

```js
class Form2 extends Component {
  renderForm = ({ handleSubmit }) => {
    ...fancy rendering stuff
  };

  render() {
    return <Form render={this.renderForm} />;
  }
}
```

This works fine, as long as `renderForm` is a pure function, solely depending on it's own parameters. However, as soon as you mix in state or props from the wrapping component (`Form2` in the example), the `renderForm` function passed to `<Form>` will become stale.

This is all much better explained in the excellent article by @ryanflorence: https://cdb.reacttraining.com/react-inline-functions-and-performance-bdff784f5578

I think the quote most relevant for this issue is the following:
> That means an inline render prop function won’t cause problems with `shouldComponentUpdate`: It can’t ever know enough to be a `PureComponent`.

As I understand it, if you use render props, you should never extend `PureComponent`, since you don't know what's going on inside the render function.

What are your thoughts?

Pull request:
Stopped extending React.PureComponent
Fixes #150.

It did require changing some tests because some of the components rendered more often than before, but this is still the right move for all the reasons described in #150.

Patch:
diff --git a/src/Field.js b/src/Field.js
index 714546d..6b34dfa 100644
--- a/src/Field.js
+++ b/src/Field.js
@@ -19,7 +19,7 @@ type State = {
   state: FieldState
 }
 
-export default class Field extends React.PureComponent<Props, State> {
+export default class Field extends React.Component<Props, State> {
   context: ReactContext
   props: Props
   state: State
diff --git a/src/FormSpy.js b/src/FormSpy.js
index 9654a4b..4a7ad01 100644
--- a/src/FormSpy.js
+++ b/src/FormSpy.js
@@ -11,7 +11,7 @@ import { all } from './ReactFinalForm'
 
 type State = { state: FormState }
 
-export default class FormSpy extends React.PureComponent<Props, State> {
+export default class FormSpy extends React.Component<Props, State> {
   context: ReactContext
   props: Props
   state: State
diff --git a/src/ReactFinalForm.js b/src/ReactFinalForm.js
index be1aae0..f0d5553 100644
--- a/src/ReactFinalForm.js
+++ b/src/ReactFinalForm.js
@@ -36,7 +36,7 @@ type State = {
   state: FormState
 }
 
-export default class ReactFinalForm extends React.PureComponent<Props, State> {
+export default class ReactFinalForm extends React.Component<Props, State> {
   context: ReactContext
   props: Props
   state: State


Test patch:
diff --git a/src/Field.test.js b/src/Field.test.js
index 449f247..424ee47 100644
--- a/src/Field.test.js
+++ b/src/Field.test.js
@@ -39,14 +39,14 @@ describe('Field', () => {
     expect(renderInput).not.toHaveBeenCalled()
     const dom = TestUtils.renderIntoDocument(<Container />)
     expect(renderInput).toHaveBeenCalled()
-    expect(renderInput).toHaveBeenCalledTimes(1)
-    expect(renderInput.mock.calls[0][0].input.value).toBe('Odie')
+    expect(renderInput).toHaveBeenCalledTimes(2)
+    expect(renderInput.mock.calls[1][0].input.value).toBe('Odie')
 
     const button = TestUtils.findRenderedDOMComponentWithTag(dom, 'button')
     TestUtils.Simulate.click(button)
 
-    expect(renderInput).toHaveBeenCalledTimes(2)
-    expect(renderInput.mock.calls[1][0].input.value).toBe('Garfield')
+    expect(renderInput).toHaveBeenCalledTimes(4)
+    expect(renderInput.mock.calls[3][0].input.value).toBe('Garfield')
   })
 
   it('should not resubscribe if name changes when not inside a <Form> (duh)', () => {
@@ -97,7 +97,7 @@ describe('Field', () => {
     // called twice due to field registration adding touched and visited values
     expect(render).toHaveBeenCalledTimes(2)
     expect(renderInput).toHaveBeenCalled()
-    expect(renderInput).toHaveBeenCalledTimes(1)
+    expect(renderInput).toHaveBeenCalledTimes(2)
   })
 
   it('should unsubscribe on unmount', () => {
@@ -245,20 +245,20 @@ describe('Field', () => {
     expect(render.mock.calls[1][0].values.foo).toBeUndefined()
 
     expect(format).toHaveBeenCalled()
-    expect(format).toHaveBeenCalledTimes(1)
+    expect(format).toHaveBeenCalledTimes(2)
     expect(format.mock.calls[0]).toEqual([undefined, 'foo'])
 
     expect(renderInput).toHaveBeenCalled()
-    expect(renderInput).toHaveBeenCalledTimes(1)
+    expect(renderInput).toHaveBeenCalledTimes(2)
     expect(renderInput.mock.calls[0][0].input.value).toBe('format.undefined')
 
     renderInput.mock.calls[0][0].input.onChange('bar')
 
-    expect(format).toHaveBeenCalledTimes(2)
-    expect(format.mock.calls[1]).toEqual(['bar', 'foo'])
+    expect(format).toHaveBeenCalledTimes(4)
+    expect(format.mock.calls[3]).toEqual(['bar', 'foo'])
 
-    expect(renderInput).toHaveBeenCalledTimes(2)
-    expect(renderInput.mock.calls[1][0].input.value).toBe('format.bar')
+    expect(renderInput).toHaveBeenCalledTimes(4)
+    expect(renderInput.mock.calls[3][0].input.value).toBe('format.bar')
   })
 
   it('should accept a null format prop to preserve undefined values', () => {
@@ -281,13 +281,13 @@ describe('Field', () => {
     expect(render.mock.calls[1][0].values.foo).toBeUndefined()
 
     expect(renderInput).toHaveBeenCalled()
-    expect(renderInput).toHaveBeenCalledTimes(1)
-    expect(renderInput.mock.calls[0][0].input.value).toBeUndefined()
+    expect(renderInput).toHaveBeenCalledTimes(2)
+    expect(renderInput.mock.calls[1][0].input.value).toBeUndefined()
 
     renderInput.mock.calls[0][0].input.onChange('bar')
 
-    expect(renderInput).toHaveBeenCalledTimes(2)
-    expect(renderInput.mock.calls[1][0].input.value).toBe('bar')
+    expect(renderInput).toHaveBeenCalledTimes(4)
+    expect(renderInput.mock.calls[3][0].input.value).toBe('bar')
   })
 
   it('should provide a value of [] when empty on a select multiple', () => {
@@ -322,18 +322,18 @@ describe('Field', () => {
     )
 
     expect(renderInput).toHaveBeenCalled()
-    expect(renderInput).toHaveBeenCalledTimes(1)
-    expect(renderInput.mock.calls[0][0].input.value).toBe('')
+    expect(renderInput).toHaveBeenCalledTimes(2)
+    expect(renderInput.mock.calls[1][0].input.value).toBe('')
 
     renderInput.mock.calls[0][0].input.onChange('bar')
 
-    expect(renderInput).toHaveBeenCalledTimes(2)
-    expect(renderInput.mock.calls[1][0].input.value).toBe('bar')
+    expect(renderInput).toHaveBeenCalledTimes(4)
+    expect(renderInput.mock.calls[3][0].input.value).toBe('bar')
 
     renderInput.mock.calls[1][0].input.onChange(null)
 
-    expect(renderInput).toHaveBeenCalledTimes(3)
-    expect(renderInput.mock.calls[2][0].input.value).toBe('')
+    expect(renderInput).toHaveBeenCalledTimes(6)
+    expect(renderInput.mock.calls[5][0].input.value).toBe('')
   })
 
   it('should optionally allow null values', () => {
@@ -451,22 +451,22 @@ describe('Field', () => {
     onChange('hi')
 
     // valid now
-    expect(input).toHaveBeenCalledTimes(3)
-    expect(input.mock.calls[2][0].meta.error).toBeUndefined()
+    expect(input).toHaveBeenCalledTimes(4)
+    expect(input.mock.calls[3][0].meta.error).toBeUndefined()
 
     // toggle rules
     const button = TestUtils.findRenderedDOMComponentWithTag(dom, 'button')
     TestUtils.Simulate.click(button)
 
     // props changed, but still valid. doesn't update until next time validation is run
-    expect(input).toHaveBeenCalledTimes(4)
-    expect(input.mock.calls[3][0].meta.error).toBeUndefined()
+    expect(input).toHaveBeenCalledTimes(5)
+    expect(input.mock.calls[4][0].meta.error).toBeUndefined()
 
     onChange('his')
 
     // invalid now
-    expect(input).toHaveBeenCalledTimes(5)
-    expect(input.mock.calls[4][0].meta.error).toBe('Must be uppercase')
+    expect(input).toHaveBeenCalledTimes(7)
+    expect(input.mock.calls[6][0].meta.error).toBe('Must be uppercase')
   })
 
   it('should render checkboxes with checked prop', () => {
diff --git a/src/FormSpy.test.js b/src/FormSpy.test.js
index 5d670b8..f644b98 100644
--- a/src/FormSpy.test.js
+++ b/src/FormSpy.test.js
@@ -47,24 +47,24 @@ describe('FormSpy', () => {
     expect(render.mock.calls[1][0].validating).toBe(false)
     expect(render.mock.calls[1][0].values).toEqual({})
     expect(renderInput).toHaveBeenCalled()
-    expect(renderInput).toHaveBeenCalledTimes(1)
+    expect(renderInput).toHaveBeenCalledTimes(2)
 
     // change value
     renderInput.mock.calls[0][0].input.onChange('bar')
 
-    expect(render).toHaveBeenCalledTimes(3)
-    hasFormApi(render.mock.calls[2][0])
-    expect(render.mock.calls[2][0].dirty).toBe(true)
-    expect(render.mock.calls[2][0].errors).toEqual({})
-    expect(render.mock.calls[2][0].invalid).toBe(false)
-    expect(render.mock.calls[2][0].pristine).toBe(false)
-    expect(render.mock.calls[2][0].submitFailed).toBe(false)
-    expect(render.mock.calls[2][0].submitSucceeded).toBe(false)
-    expect(render.mock.calls[2][0].submitting).toBe(false)
-    expect(render.mock.calls[2][0].valid).toBe(true)
-    expect(render.mock.calls[2][0].validating).toBe(false)
-    expect(render.mock.calls[2][0].values).toEqual({ foo: 'bar' })
-    expect(renderInput).toHaveBeenCalledTimes(2)
+    expect(render).toHaveBeenCalledTimes(4)
+    hasFormApi(render.mock.calls[3][0])
+    expect(render.mock.calls[3][0].dirty).toBe(true)
+    expect(render.mock.calls[3][0].errors).toEqual({})
+    expect(render.mock.calls[3][0].invalid).toBe(false)
+    expect(render.mock.calls[3][0].pristine).toBe(false)
+    expect(render.mock.calls[3][0].submitFailed).toBe(false)
+    expect(render.mock.calls[3][0].submitSucceeded).toBe(false)
+    expect(render.mock.calls[3][0].submitting).toBe(false)
+    expect(render.mock.calls[3][0].valid).toBe(true)
+    expect(render.mock.calls[3][0].validating).toBe(false)
+    expect(render.mock.calls[3][0].values).toEqual({ foo: 'bar' })
+    expect(renderInput).toHaveBeenCalledTimes(4)
   })
 
   it('should resubscribe if subscription changes', () => {
@@ -164,7 +164,7 @@ describe('FormSpy', () => {
     expect(render.mock.calls[1][0].validating).toBeUndefined()
     expect(render.mock.calls[1][0].values).toEqual({})
     expect(renderInput).toHaveBeenCalled()
-    expect(renderInput).toHaveBeenCalledTimes(1)
+    expect(renderInput).toHaveBeenCalledTimes(2)
 
     // change value
     renderInput.mock.calls[0][0].input.onChange('bar')
@@ -182,7 +182,7 @@ describe('FormSpy', () => {
     expect(render.mock.calls[3][0].valid).toBeUndefined()
     expect(render.mock.calls[3][0].validating).toBeUndefined()
     expect(render.mock.calls[3][0].values).toEqual({ foo: 'bar' })
-    expect(renderInput).toHaveBeenCalledTimes(2)
+    expect(renderInput).toHaveBeenCalledTimes(4)
   })
 
   it('should not unsubscribe/resubscribe if not in form', () => {
@@ -259,14 +259,14 @@ describe('FormSpy', () => {
       </Form>
     )
     expect(input).toHaveBeenCalled()
-    expect(input).toHaveBeenCalledTimes(1)
+    expect(input).toHaveBeenCalledTimes(2)
     expect(onChange).toHaveBeenCalled()
     expect(onChange).toHaveBeenCalledTimes(1)
     expect(onChange).toHaveBeenCalledWith({ dirty: false })
 
     input.mock.calls[0][0].input.onChange('bar')
 
-    expect(input).toHaveBeenCalledTimes(2)
+    expect(input).toHaveBeenCalledTimes(4)
     expect(onChange).toHaveBeenCalledTimes(2)
     expect(onChange).toHaveBeenCalledWith({ dirty: true })
   })